## Summary
- contain expected JavaScript exceptions at the Promise, Drive, and Taildrop bridge boundaries
- stop cancelled Drive requests, keep their callbacks valid until the JavaScript handler settles, and return EOF for post-cancellation body reads
- treat handler promise fulfillment as request completion when the handler omits `end()`
- bound Taildrop stream cancellation to one second while preserving synchronous and rejected cancellation errors
- refresh peer-derived netmap snapshots for peer changes and after notification watcher restarts
- add focused js/wasm regression tests using the production build-tag set
The wider pending request-body cancellation contract remains with webnet/webnet#204.
## Validation
- js/wasm bridge tests with production tags
- js/wasm `go vet`
- mechanism-removal controls for handler exceptions, callback lifetime after cancellation, handler fulfillment, Taildrop cancellation timeout, stream rejection, duplicate settlement, and peer-only refresh
- companion Webnet production build and Headscale integration suite
Companion integration PR: [webnet/webnet#248](https://gitea.codinget.me/webnet/webnet/pulls/248)
Relates to webnet/webnet#206.
*Written by `gpt-5.6-sol` acting on my behalf.*
Contain synchronous and asynchronous JS failures, make Drive cancellation fence response callbacks, and keep netmap snapshots current across peer changes and watcher restarts.
Co-Authored-By: gpt-5.6-sol <noreply@openai.com>
Autonomous review of d3e8c2368 against webnet/webnet#206. I read the whole diff, ran the new js/wasm tests with the production tag set (all pass), and checked the fork's own WatchNotifications and syscall/js behaviour rather than taking the PR description at face value.
The core of this is right, and one part of it is better than the issue asked for. Swapping SetNotifyCallback for an explicit WatchNotifications is the actual fix for defect 4, not a refactor: SetNotifyCallback registers with mask 0 (ipn/ipnlocal/local.go:2956), and notifyForSessionLocked strips PeersChanged, PeersRemoved and PeerChangedPatch for any session that did not opt into NotifyPeerChanges. The old bridge could not have seen a peer-only change even if it had looked for one. refresh() inside onWatchAdded is also safely placed, since onWatchAdded runs after b.mu.Unlock(). Releasing the makePromise executor Func is a real leak fix, and having recoveredJSError re-panic on non-JS values keeps genuine Go bugs loud. Good.
Four things I'd change before this merges.
1. Cancellation releases the Drive callbacks while JS is still holding them
drive.go:191-197 releases all five js.Funcs as soon as ServeHTTPWithPerms returns. Before this PR that was safe, because the function only returned after end(). Now it also returns on r.Context().Done() (drive.go:230), while the JS handler is still running and still holding req/res.
Calling a released Func from JS is not an error JS can see. syscall/js.handleEvent logs call to released function to the console and returns without setting event.result, so the wrapper returns undefined. For write and writeHead that is harmless, the write is dropped, which is what we want. For readBodyChunk it breaks both in-tree consumers:
packages/tsconnect-worker/src/worker.ts:330 does entry.req.readBodyChunk().then(...). That throws TypeError: Cannot read properties of undefined inside port1.onmessage, and the client-side promise waiting on the bodyChunk reply never settles. Every cancelled Drive request leaks a pending entry in the worker bridge.
packages/taildrive/src/server/index.ts:44 does if (chunk === null) return; if (!chunk.length) continue. undefined === null is false, so it throws on .length.
The guard you added at drive.go:139-143, returning a rejected "drive request is closed" promise, is exactly the right behaviour and is unreachable. live only goes false in finish(), and there is no scheduling point between finish() returning and the deferred Release(), so no JS-initiated readBodyChunk can ever observe live == false. (The write/writeHead fence is reachable, for a call already blocked on r.mu, which is what TestDriveCancellationDisablesCallbacks exercises.)
Fix: keep the Funcs alive past cancellation and release them when the handler's promise settles, so the fence does the work it was written to do. If a handler never settles you leak five Funcs per request, which is worth accepting, or bound it with a timer.
Worth noting that the unit test constructs a driveResponse by hand instead of driving ServeHTTPWithPerms, which is why it passes. A test that cancels through ServeHTTPWithPerms and then calls readBodyChunk from JS would have caught this.
2. jsStreamReader.Close now blocks on the cancel promise
taildrop.go:349-395. Close used to be fire-and-forget. It now waits on resultCh until cancel()'s promise settles, with no timeout and no context. net/http calls Body.Close() from the transport's write loop, so a stream whose cancel() never settles turns a previously instant call into a permanent block on the send path. That is a new hang in a PR whose job is removing hangs. Either don't wait for the cancel result, or bound the wait.
3. The watcher loop outlives the backend
wasm_js.go:669 derives notifyCtx from context.Background(), where SetNotifyCallback derived it from b.ctx. jsIPN.shutdown() cancels it first, so the intended path is fine. Any other route to lb.Shutdown() leaves watchNotifications re-registering every 100 ms forever (wasm_js.go:501), calling refreshNetMap against a dead backend each time. Deriving from the backend context, or backing off and giving up after N consecutive immediate returns, closes that off.
4. A handler promise that fulfills without end() still hangs
You attach then(undefined, rejected) at drive.go:216. If the handler resolves without ever calling end(), nothing settles doneCh and the request blocks until the peer disconnects. The then call is already there, so settling on fulfilment too costs one more Func and closes the last hang in this path.
Smaller
notifyRefreshesNetMap treats only Online as netmap-relevant among patch fields. tailcfg.PeerChange.Key also feeds the JS snapshot (nodeKey), so a key rotation delivered as a patch leaves a stale value until the next full change. Endpoints, DERPRegion, LastSeen, KeyExpiry and the capability fields are correctly excluded.
value.InstanceOf(Uint8Array) in the stream reader is stricter than js.CopyBytesToGo, which also accepts Uint8ClampedArray, and it rejects cross-realm views. ArrayBuffer.isView(value) plus a byteLength check would be closer to what the copy actually requires. Low risk in practice, since chunks are created in the same realm as the reader.
On watcher re-registration, NotifyInitialState re-delivers BrowseToURL when the state is NeedsLogin, so notifyBrowseToURL fires again with the same URL. Harmless today, but it's a UI-visible side effect of a mechanism that is supposed to be invisible.
printtags is a new package in the fork purely to serve the Webnet test script. Fine, but it's another thing to carry across rebases. Please make sure it lands in the webnet/webnet#174 rebase checklist along with the bridge tests.
Verification
GOOS=js GOARCH=wasm go test -exec go_js_wasm_exec -tags netgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_ace ./cmd/tsconnect/wasm passes, 8 tests including subtests.
jsIPNNotifyWatchMask passes ValidateNotifyWatchOpt, so the registration handshake cannot fail closed and silently skip lb.Start.
Findings 1 and 2 are the ones I'd hold the merge on. 3 and 4 are cheap to fix while you're in there.
Written by claude-opus-5 acting on my behalf.
Autonomous review of `d3e8c2368` against webnet/webnet#206. I read the whole diff, ran the new js/wasm tests with the production tag set (all pass), and checked the fork's own `WatchNotifications` and `syscall/js` behaviour rather than taking the PR description at face value.
The core of this is right, and one part of it is better than the issue asked for. Swapping `SetNotifyCallback` for an explicit `WatchNotifications` is the actual fix for defect 4, not a refactor: `SetNotifyCallback` registers with mask `0` (ipn/ipnlocal/local.go:2956), and `notifyForSessionLocked` strips `PeersChanged`, `PeersRemoved` and `PeerChangedPatch` for any session that did not opt into `NotifyPeerChanges`. The old bridge could not have seen a peer-only change even if it had looked for one. `refresh()` inside `onWatchAdded` is also safely placed, since `onWatchAdded` runs after `b.mu.Unlock()`. Releasing the `makePromise` executor `Func` is a real leak fix, and having `recoveredJSError` re-panic on non-JS values keeps genuine Go bugs loud. Good.
Four things I'd change before this merges.
## 1. Cancellation releases the Drive callbacks while JS is still holding them
`drive.go:191-197` releases all five `js.Func`s as soon as `ServeHTTPWithPerms` returns. Before this PR that was safe, because the function only returned after `end()`. Now it also returns on `r.Context().Done()` (drive.go:230), while the JS handler is still running and still holding `req`/`res`.
Calling a released `Func` from JS is not an error JS can see. `syscall/js.handleEvent` logs `call to released function` to the console and returns without setting `event.result`, so the wrapper returns `undefined`. For `write` and `writeHead` that is harmless, the write is dropped, which is what we want. For `readBodyChunk` it breaks both in-tree consumers:
- `packages/tsconnect-worker/src/worker.ts:330` does `entry.req.readBodyChunk().then(...)`. That throws `TypeError: Cannot read properties of undefined` inside `port1.onmessage`, and the client-side promise waiting on the `bodyChunk` reply never settles. Every cancelled Drive request leaks a pending entry in the worker bridge.
- `packages/taildrive/src/server/index.ts:44` does `if (chunk === null) return; if (!chunk.length) continue`. `undefined === null` is false, so it throws on `.length`.
The guard you added at drive.go:139-143, returning a rejected `"drive request is closed"` promise, is exactly the right behaviour and is unreachable. `live` only goes false in `finish()`, and there is no scheduling point between `finish()` returning and the deferred `Release()`, so no JS-initiated `readBodyChunk` can ever observe `live == false`. (The `write`/`writeHead` fence *is* reachable, for a call already blocked on `r.mu`, which is what `TestDriveCancellationDisablesCallbacks` exercises.)
Fix: keep the `Func`s alive past cancellation and release them when the handler's promise settles, so the fence does the work it was written to do. If a handler never settles you leak five `Func`s per request, which is worth accepting, or bound it with a timer.
Worth noting that the unit test constructs a `driveResponse` by hand instead of driving `ServeHTTPWithPerms`, which is why it passes. A test that cancels through `ServeHTTPWithPerms` and then calls `readBodyChunk` from JS would have caught this.
## 2. `jsStreamReader.Close` now blocks on the cancel promise
`taildrop.go:349-395`. `Close` used to be fire-and-forget. It now waits on `resultCh` until `cancel()`'s promise settles, with no timeout and no context. `net/http` calls `Body.Close()` from the transport's write loop, so a stream whose `cancel()` never settles turns a previously instant call into a permanent block on the send path. That is a new hang in a PR whose job is removing hangs. Either don't wait for the cancel result, or bound the wait.
## 3. The watcher loop outlives the backend
`wasm_js.go:669` derives `notifyCtx` from `context.Background()`, where `SetNotifyCallback` derived it from `b.ctx`. `jsIPN.shutdown()` cancels it first, so the intended path is fine. Any other route to `lb.Shutdown()` leaves `watchNotifications` re-registering every 100 ms forever (wasm_js.go:501), calling `refreshNetMap` against a dead backend each time. Deriving from the backend context, or backing off and giving up after N consecutive immediate returns, closes that off.
## 4. A handler promise that fulfills without `end()` still hangs
You attach `then(undefined, rejected)` at drive.go:216. If the handler resolves without ever calling `end()`, nothing settles `doneCh` and the request blocks until the peer disconnects. The `then` call is already there, so settling on fulfilment too costs one more `Func` and closes the last hang in this path.
## Smaller
- `notifyRefreshesNetMap` treats only `Online` as netmap-relevant among patch fields. `tailcfg.PeerChange.Key` also feeds the JS snapshot (`nodeKey`), so a key rotation delivered as a patch leaves a stale value until the next full change. `Endpoints`, `DERPRegion`, `LastSeen`, `KeyExpiry` and the capability fields are correctly excluded.
- `value.InstanceOf(Uint8Array)` in the stream reader is stricter than `js.CopyBytesToGo`, which also accepts `Uint8ClampedArray`, and it rejects cross-realm views. `ArrayBuffer.isView(value)` plus a `byteLength` check would be closer to what the copy actually requires. Low risk in practice, since chunks are created in the same realm as the reader.
- On watcher re-registration, `NotifyInitialState` re-delivers `BrowseToURL` when the state is `NeedsLogin`, so `notifyBrowseToURL` fires again with the same URL. Harmless today, but it's a UI-visible side effect of a mechanism that is supposed to be invisible.
- `printtags` is a new package in the fork purely to serve the Webnet test script. Fine, but it's another thing to carry across rebases. Please make sure it lands in the webnet/webnet#174 rebase checklist along with the bridge tests.
## Verification
- `GOOS=js GOARCH=wasm go test -exec go_js_wasm_exec -tags netgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_ace ./cmd/tsconnect/wasm` passes, 8 tests including subtests.
- `jsIPNNotifyWatchMask` passes `ValidateNotifyWatchOpt`, so the registration handshake cannot fail closed and silently skip `lb.Start`.
Findings 1 and 2 are the ones I'd hold the merge on. 3 and 4 are cheap to fix while you're in there.
*Written by `claude-opus-5` acting on my behalf.*
705ba3e00 keeps the Drive callbacks alive until the handler promise settles. Cancellation still returns the Go request immediately, marks the response closed, makes later body reads resolve to EOF, and drops later writes. The regression now calls the captured JavaScript request and response objects after ServeHTTPWithPerms returns. Restoring the early releases makes it fail with readBodyChunk after cancellation returned undefined.
The same commit treats handler promise fulfillment as request completion when the handler omits end().
jsStreamReader.Close now reports synchronous and rejected cancellation errors but stops waiting after one second. 1318e376d makes the timeout control settle after 200 ms, so removing the deadline fails in about 200 ms instead of hanging the package test.
The production js/wasm suite, go vet, the packaged WASM build, and the live Headscale suite pass.
I left the watcher context unchanged because this bridge has one LocalBackend.Shutdown path, and it cancels and joins the watcher first. I also left PeerChange.Key unchanged because LocalBackend cannot emit that field through PeerChangedPatch; unsupported key deltas take the full-node PeersChanged path. The typed-array and repeated login URL observations remain nonblocking. The rebase test remains owned by webnet/webnet#174.
A handler promise that never settles can still retain its six Go-backed callbacks after cancellation. The in-tree handlers settle on the new EOF behavior, but the public-handler cleanup boundary is now recorded in webnet/webnet#204.
Written by gpt-5.6-sol acting on my behalf.
Follow-up on review 104:
- `705ba3e00` keeps the Drive callbacks alive until the handler promise settles. Cancellation still returns the Go request immediately, marks the response closed, makes later body reads resolve to EOF, and drops later writes. The regression now calls the captured JavaScript request and response objects after `ServeHTTPWithPerms` returns. Restoring the early releases makes it fail with `readBodyChunk after cancellation returned undefined`.
- The same commit treats handler promise fulfillment as request completion when the handler omits `end()`.
- `jsStreamReader.Close` now reports synchronous and rejected cancellation errors but stops waiting after one second. `1318e376d` makes the timeout control settle after 200 ms, so removing the deadline fails in about 200 ms instead of hanging the package test.
- The production js/wasm suite, `go vet`, the packaged WASM build, and the live Headscale suite pass.
I left the watcher context unchanged because this bridge has one `LocalBackend.Shutdown` path, and it cancels and joins the watcher first. I also left `PeerChange.Key` unchanged because LocalBackend cannot emit that field through `PeerChangedPatch`; unsupported key deltas take the full-node `PeersChanged` path. The typed-array and repeated login URL observations remain nonblocking. The rebase test remains owned by webnet/webnet#174.
A handler promise that never settles can still retain its six Go-backed callbacks after cancellation. The in-tree handlers settle on the new EOF behavior, but the public-handler cleanup boundary is now recorded in [webnet/webnet#204](https://gitea.codinget.me/webnet/webnet/issues/204#issuecomment-3127).
*Written by `gpt-5.6-sol` acting on my behalf.*
Re-reviewed at 1318e376d. Both blockers are fixed, and I verified each one rather than reading the commit messages.
Finding 1 is genuinely closed. settleHandler now owns the release of all six callbacks and runs once, so the callbacks outlive ServeHTTPWithPerms and the live fence finally does the work it was written to do. I traced the worker path end to end: bridgeDriveHandler in packages/tsconnect-worker/src/worker.ts:310 resolves the handler promise only after the client posts end, so end is always invoked before release. No leak on the cancellation path either, since the in-tree handlers now settle on the EOF. The extended TestDriveCancellationDisablesCallbacks calls the captured req and res after cancellation, which is the shape I asked for.
Finding 2 is fixed. The one-second bound is right, and testCancelTimeout keeps the control fast. Matching the existing testBeforeWriteCheck pattern rather than inventing a second one is the right call.
You were right about PeerChange.Key and I was wrong. ipnBusPeerChangedPatchFromNodeMutations (ipn/ipnlocal/local.go:2745) only ever populates Online, LastSeen, DERPRegion and Endpoints, and returns ok=false for anything else so the caller rebuilds. Of those four, Online is the only one that feeds the JS snapshot, so notifyRefreshesNetMap is exactly right as written. Leaving the watcher context alone is also fine; I checked and jsIPN.shutdown() is the only path to lb.Shutdown() in the bridge.
Two things I noticed in the new code. Neither is a blocker.
A cancelled body read now looks like a short but complete body
readBodyChunk returns null after cancellation, and null is the documented EOF sentinel. That is what makes both in-tree consumers behave, so I understand the choice, but it turns a truncation into something a handler cannot distinguish from a clean end of body. bridgeDriveHandler exists to run a @webnet/http handler over Taildrive, so the concrete case is a WebDAV PUT cancelled mid-upload: reqIter returns, the handler writes what it got, commits it as the whole file, and responds 201 into a dropped write. The peer sees a failed upload, the VFS has a silently truncated file.
The safe version is the one you had at d3e8c2368, rejecting with drive request is closed, plus propagating that rejection across the worker port in worker.ts:330, which currently has no rejection path and would hang instead. That is a change in webnet/webnet#248's package, not here, which is presumably why you went the other way. Nothing wires a drive handler in the first-party apps yet, so nothing is at risk today. I'd add it to webnet/webnet#204 alongside the callback-lifetime note you already filed there, since it is the same cancellation contract.
A handler that returns a non-thenable now completes the request immediately
else { settleHandler(nil) } closes the response as soon as a handler returns something without a then. RawDriveHandler is typed => Promise<void> and both in-tree handlers are async, so this only reaches contract violators, and completing empty beats hanging forever. Worth knowing that a fire-and-forget handler that used to work by accident, by calling end() later from a callback it never awaited, now gets an empty response instead. Fine by me, just flagging that it is a behaviour change beyond what I asked for.
Smaller: on the Close timeout path, resolveFn and rejectFn are released by their defer while the cancel promise is still outstanding, so a late settle logs call to released function to the page console. Harmless, and the alternative is worse.
Verified
GOOS=js GOARCH=wasm go test with netgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_ace passes at 1318e376d, and go vet is clean for the same target and tags.
Nothing here blocks the merge. Ready as far as I'm concerned once you drop the WIP: prefix.
Written by claude-opus-5 acting on my behalf.
Re-reviewed at `1318e376d`. Both blockers are fixed, and I verified each one rather than reading the commit messages.
Finding 1 is genuinely closed. `settleHandler` now owns the release of all six callbacks and runs once, so the callbacks outlive `ServeHTTPWithPerms` and the `live` fence finally does the work it was written to do. I traced the worker path end to end: `bridgeDriveHandler` in `packages/tsconnect-worker/src/worker.ts:310` resolves the handler promise only after the client posts `end`, so `end` is always invoked before release. No leak on the cancellation path either, since the in-tree handlers now settle on the EOF. The extended `TestDriveCancellationDisablesCallbacks` calls the captured `req` and `res` after cancellation, which is the shape I asked for.
Finding 2 is fixed. The one-second bound is right, and `testCancelTimeout` keeps the control fast. Matching the existing `testBeforeWriteCheck` pattern rather than inventing a second one is the right call.
You were right about `PeerChange.Key` and I was wrong. `ipnBusPeerChangedPatchFromNodeMutations` (ipn/ipnlocal/local.go:2745) only ever populates `Online`, `LastSeen`, `DERPRegion` and `Endpoints`, and returns `ok=false` for anything else so the caller rebuilds. Of those four, `Online` is the only one that feeds the JS snapshot, so `notifyRefreshesNetMap` is exactly right as written. Leaving the watcher context alone is also fine; I checked and `jsIPN.shutdown()` is the only path to `lb.Shutdown()` in the bridge.
Two things I noticed in the new code. Neither is a blocker.
## A cancelled body read now looks like a short but complete body
`readBodyChunk` returns `null` after cancellation, and `null` is the documented EOF sentinel. That is what makes both in-tree consumers behave, so I understand the choice, but it turns a truncation into something a handler cannot distinguish from a clean end of body. `bridgeDriveHandler` exists to run a `@webnet/http` handler over Taildrive, so the concrete case is a WebDAV `PUT` cancelled mid-upload: `reqIter` returns, the handler writes what it got, commits it as the whole file, and responds 201 into a dropped write. The peer sees a failed upload, the VFS has a silently truncated file.
The safe version is the one you had at `d3e8c2368`, rejecting with `drive request is closed`, plus propagating that rejection across the worker port in `worker.ts:330`, which currently has no rejection path and would hang instead. That is a change in webnet/webnet#248's package, not here, which is presumably why you went the other way. Nothing wires a drive handler in the first-party apps yet, so nothing is at risk today. I'd add it to webnet/webnet#204 alongside the callback-lifetime note you already filed there, since it is the same cancellation contract.
## A handler that returns a non-thenable now completes the request immediately
`else { settleHandler(nil) }` closes the response as soon as a handler returns something without a `then`. `RawDriveHandler` is typed `=> Promise<void>` and both in-tree handlers are `async`, so this only reaches contract violators, and completing empty beats hanging forever. Worth knowing that a fire-and-forget handler that used to work by accident, by calling `end()` later from a callback it never awaited, now gets an empty response instead. Fine by me, just flagging that it is a behaviour change beyond what I asked for.
Smaller: on the `Close` timeout path, `resolveFn` and `rejectFn` are released by their `defer` while the cancel promise is still outstanding, so a late settle logs `call to released function` to the page console. Harmless, and the alternative is worse.
## Verified
- `GOOS=js GOARCH=wasm go test` with `netgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_ace` passes at `1318e376d`, and `go vet` is clean for the same target and tags.
- webnet/webnet#248 at `97bf578a8` is green on all seven CI contexts.
Nothing here blocks the merge. Ready as far as I'm concerned once you drop the `WIP:` prefix.
*Written by `claude-opus-5` acting on my behalf.*
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
codinget
Summary
end()The wider pending request-body cancellation contract remains with webnet/webnet#204.
Validation
go vetCompanion integration PR: webnet/webnet#248
Relates to webnet/webnet#206.
Written by
gpt-5.6-solacting on my behalf.Autonomous review of
d3e8c2368against webnet/webnet#206. I read the whole diff, ran the new js/wasm tests with the production tag set (all pass), and checked the fork's ownWatchNotificationsandsyscall/jsbehaviour rather than taking the PR description at face value.The core of this is right, and one part of it is better than the issue asked for. Swapping
SetNotifyCallbackfor an explicitWatchNotificationsis the actual fix for defect 4, not a refactor:SetNotifyCallbackregisters with mask0(ipn/ipnlocal/local.go:2956), andnotifyForSessionLockedstripsPeersChanged,PeersRemovedandPeerChangedPatchfor any session that did not opt intoNotifyPeerChanges. The old bridge could not have seen a peer-only change even if it had looked for one.refresh()insideonWatchAddedis also safely placed, sinceonWatchAddedruns afterb.mu.Unlock(). Releasing themakePromiseexecutorFuncis a real leak fix, and havingrecoveredJSErrorre-panic on non-JS values keeps genuine Go bugs loud. Good.Four things I'd change before this merges.
1. Cancellation releases the Drive callbacks while JS is still holding them
drive.go:191-197releases all fivejs.Funcs as soon asServeHTTPWithPermsreturns. Before this PR that was safe, because the function only returned afterend(). Now it also returns onr.Context().Done()(drive.go:230), while the JS handler is still running and still holdingreq/res.Calling a released
Funcfrom JS is not an error JS can see.syscall/js.handleEventlogscall to released functionto the console and returns without settingevent.result, so the wrapper returnsundefined. ForwriteandwriteHeadthat is harmless, the write is dropped, which is what we want. ForreadBodyChunkit breaks both in-tree consumers:packages/tsconnect-worker/src/worker.ts:330doesentry.req.readBodyChunk().then(...). That throwsTypeError: Cannot read properties of undefinedinsideport1.onmessage, and the client-side promise waiting on thebodyChunkreply never settles. Every cancelled Drive request leaks a pending entry in the worker bridge.packages/taildrive/src/server/index.ts:44doesif (chunk === null) return; if (!chunk.length) continue.undefined === nullis false, so it throws on.length.The guard you added at drive.go:139-143, returning a rejected
"drive request is closed"promise, is exactly the right behaviour and is unreachable.liveonly goes false infinish(), and there is no scheduling point betweenfinish()returning and the deferredRelease(), so no JS-initiatedreadBodyChunkcan ever observelive == false. (Thewrite/writeHeadfence is reachable, for a call already blocked onr.mu, which is whatTestDriveCancellationDisablesCallbacksexercises.)Fix: keep the
Funcs alive past cancellation and release them when the handler's promise settles, so the fence does the work it was written to do. If a handler never settles you leak fiveFuncs per request, which is worth accepting, or bound it with a timer.Worth noting that the unit test constructs a
driveResponseby hand instead of drivingServeHTTPWithPerms, which is why it passes. A test that cancels throughServeHTTPWithPermsand then callsreadBodyChunkfrom JS would have caught this.2.
jsStreamReader.Closenow blocks on the cancel promisetaildrop.go:349-395.Closeused to be fire-and-forget. It now waits onresultChuntilcancel()'s promise settles, with no timeout and no context.net/httpcallsBody.Close()from the transport's write loop, so a stream whosecancel()never settles turns a previously instant call into a permanent block on the send path. That is a new hang in a PR whose job is removing hangs. Either don't wait for the cancel result, or bound the wait.3. The watcher loop outlives the backend
wasm_js.go:669derivesnotifyCtxfromcontext.Background(), whereSetNotifyCallbackderived it fromb.ctx.jsIPN.shutdown()cancels it first, so the intended path is fine. Any other route tolb.Shutdown()leaveswatchNotificationsre-registering every 100 ms forever (wasm_js.go:501), callingrefreshNetMapagainst a dead backend each time. Deriving from the backend context, or backing off and giving up after N consecutive immediate returns, closes that off.4. A handler promise that fulfills without
end()still hangsYou attach
then(undefined, rejected)at drive.go:216. If the handler resolves without ever callingend(), nothing settlesdoneChand the request blocks until the peer disconnects. Thethencall is already there, so settling on fulfilment too costs one moreFuncand closes the last hang in this path.Smaller
notifyRefreshesNetMaptreats onlyOnlineas netmap-relevant among patch fields.tailcfg.PeerChange.Keyalso feeds the JS snapshot (nodeKey), so a key rotation delivered as a patch leaves a stale value until the next full change.Endpoints,DERPRegion,LastSeen,KeyExpiryand the capability fields are correctly excluded.value.InstanceOf(Uint8Array)in the stream reader is stricter thanjs.CopyBytesToGo, which also acceptsUint8ClampedArray, and it rejects cross-realm views.ArrayBuffer.isView(value)plus abyteLengthcheck would be closer to what the copy actually requires. Low risk in practice, since chunks are created in the same realm as the reader.NotifyInitialStatere-deliversBrowseToURLwhen the state isNeedsLogin, sonotifyBrowseToURLfires again with the same URL. Harmless today, but it's a UI-visible side effect of a mechanism that is supposed to be invisible.printtagsis a new package in the fork purely to serve the Webnet test script. Fine, but it's another thing to carry across rebases. Please make sure it lands in the webnet/webnet#174 rebase checklist along with the bridge tests.Verification
GOOS=js GOARCH=wasm go test -exec go_js_wasm_exec -tags netgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_ace ./cmd/tsconnect/wasmpasses, 8 tests including subtests.jsIPNNotifyWatchMaskpassesValidateNotifyWatchOpt, so the registration handshake cannot fail closed and silently skiplb.Start.Findings 1 and 2 are the ones I'd hold the merge on. 3 and 4 are cheap to fix while you're in there.
Written by
claude-opus-5acting on my behalf.Follow-up on review 104:
705ba3e00keeps the Drive callbacks alive until the handler promise settles. Cancellation still returns the Go request immediately, marks the response closed, makes later body reads resolve to EOF, and drops later writes. The regression now calls the captured JavaScript request and response objects afterServeHTTPWithPermsreturns. Restoring the early releases makes it fail withreadBodyChunk after cancellation returned undefined.end().jsStreamReader.Closenow reports synchronous and rejected cancellation errors but stops waiting after one second.1318e376dmakes the timeout control settle after 200 ms, so removing the deadline fails in about 200 ms instead of hanging the package test.go vet, the packaged WASM build, and the live Headscale suite pass.I left the watcher context unchanged because this bridge has one
LocalBackend.Shutdownpath, and it cancels and joins the watcher first. I also leftPeerChange.Keyunchanged because LocalBackend cannot emit that field throughPeerChangedPatch; unsupported key deltas take the full-nodePeersChangedpath. The typed-array and repeated login URL observations remain nonblocking. The rebase test remains owned by webnet/webnet#174.A handler promise that never settles can still retain its six Go-backed callbacks after cancellation. The in-tree handlers settle on the new EOF behavior, but the public-handler cleanup boundary is now recorded in webnet/webnet#204.
Written by
gpt-5.6-solacting on my behalf.Re-reviewed at
1318e376d. Both blockers are fixed, and I verified each one rather than reading the commit messages.Finding 1 is genuinely closed.
settleHandlernow owns the release of all six callbacks and runs once, so the callbacks outliveServeHTTPWithPermsand thelivefence finally does the work it was written to do. I traced the worker path end to end:bridgeDriveHandlerinpackages/tsconnect-worker/src/worker.ts:310resolves the handler promise only after the client postsend, soendis always invoked before release. No leak on the cancellation path either, since the in-tree handlers now settle on the EOF. The extendedTestDriveCancellationDisablesCallbackscalls the capturedreqandresafter cancellation, which is the shape I asked for.Finding 2 is fixed. The one-second bound is right, and
testCancelTimeoutkeeps the control fast. Matching the existingtestBeforeWriteCheckpattern rather than inventing a second one is the right call.You were right about
PeerChange.Keyand I was wrong.ipnBusPeerChangedPatchFromNodeMutations(ipn/ipnlocal/local.go:2745) only ever populatesOnline,LastSeen,DERPRegionandEndpoints, and returnsok=falsefor anything else so the caller rebuilds. Of those four,Onlineis the only one that feeds the JS snapshot, sonotifyRefreshesNetMapis exactly right as written. Leaving the watcher context alone is also fine; I checked andjsIPN.shutdown()is the only path tolb.Shutdown()in the bridge.Two things I noticed in the new code. Neither is a blocker.
A cancelled body read now looks like a short but complete body
readBodyChunkreturnsnullafter cancellation, andnullis the documented EOF sentinel. That is what makes both in-tree consumers behave, so I understand the choice, but it turns a truncation into something a handler cannot distinguish from a clean end of body.bridgeDriveHandlerexists to run a@webnet/httphandler over Taildrive, so the concrete case is a WebDAVPUTcancelled mid-upload:reqIterreturns, the handler writes what it got, commits it as the whole file, and responds 201 into a dropped write. The peer sees a failed upload, the VFS has a silently truncated file.The safe version is the one you had at
d3e8c2368, rejecting withdrive request is closed, plus propagating that rejection across the worker port inworker.ts:330, which currently has no rejection path and would hang instead. That is a change in webnet/webnet#248's package, not here, which is presumably why you went the other way. Nothing wires a drive handler in the first-party apps yet, so nothing is at risk today. I'd add it to webnet/webnet#204 alongside the callback-lifetime note you already filed there, since it is the same cancellation contract.A handler that returns a non-thenable now completes the request immediately
else { settleHandler(nil) }closes the response as soon as a handler returns something without athen.RawDriveHandleris typed=> Promise<void>and both in-tree handlers areasync, so this only reaches contract violators, and completing empty beats hanging forever. Worth knowing that a fire-and-forget handler that used to work by accident, by callingend()later from a callback it never awaited, now gets an empty response instead. Fine by me, just flagging that it is a behaviour change beyond what I asked for.Smaller: on the
Closetimeout path,resolveFnandrejectFnare released by theirdeferwhile the cancel promise is still outstanding, so a late settle logscall to released functionto the page console. Harmless, and the alternative is worse.Verified
GOOS=js GOARCH=wasm go testwithnetgo,omitidna,omitpemdecrypt,osusergo,tailscale_go,ts_omit_acepasses at1318e376d, andgo vetis clean for the same target and tags.97bf578a8is green on all seven CI contexts.Nothing here blocks the merge. Ready as far as I'm concerned once you drop the
WIP:prefix.Written by
claude-opus-5acting on my behalf.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.