On a mid-stream write error the loop breaks and releaseLock()s but never cancels the source stream, so the producer feeding it is left hanging with no signal to stop (same pattern I flagged on the FTP client). A reader.cancel(writeError) before releaseLock() on the error path would propagate cancellation. Minor.
Read pipelining assumes reads are never short except at EOF — a short mid-file READ silently drops data. fill() advances nextOffset/requested by the requested want, and the pull loop advances the stream by whatever each READ returns, without reconciling the two. If a server returns fewer than want bytes for a non-final READ (legal in the protocol; happens with pipes/special files and some non-OpenSSH servers), the already-pipelined next request sits at offset + want, so the want - returned bytes in between are never requested and the delivered file is corrupt (short, with a gap), with no error. This is safe against the bundled server (its readFromHandle always fills len until EOF) and against regular files on spec-compliant servers, which is why loopback + OpenSSH tests pass. To be robust, treat data.length < want as authoritative: set nextOffset from the actual bytes returned and discard/re-issue the outstanding pipelined requests, rather than trusting the optimistic offsets. At minimum, document the regular-files-only assumption.
Server-side STARTTLS race: an early ClientHello makes the upgrade hard-fail. After a completed read() the socket stays in flowing mode (read() resumes it and #onData with a waiting callback never re-pauses). So in the server dance — read STARTTLS, write("220 …"), upgradeTls({isServer}) — a fast client's ClientHello can arrive in the event-loop gap after the write, land in #buffers, and the upgrade then throws Cannot upgrade with buffered data. Loopback tests won't hit this, but a real peer that pipelines the handshake right behind reading the 220 will, intermittently.
Autonomous review (Claude Fable 5). The API shape is good (quiescence requirement, close-on-handshake-failure, in-flight guards), the node implementation's detach/attach listener handling is careful, and the test suite covers the edge cases well. Two real findings below — a timing race on the server side of the node transport, and an over-eager close on the worker path. The Go side of the submodule bump (webnet/tailscale#14) was not reviewed here.
upgradeError conflates validation failures with handshake failures. Conn.upgradeTls on the worker side throws for plenty of non-fatal reasons — missing serverName, options type errors, the "rebuild main.wasm" feature check — none of which close the underlying Go conn. But this branch unconditionally marks the WorkerConn closed and closes the port, so: (a) a caller who passes bad options loses a perfectly healthy connection (unlike the direct Conn, which stays usable), and (b) the worker-side conn is never actually closed — it leaks. Either mirror the cheap validations client-side before sending the message, or split the protocol reply into "failed, conn still open" vs "failed, conn closed" and only tear down on the latter.
Two small lifecycle nits: (1) if the connect promise rejected, await (await conn) makes close() reject too — worth a catch; (2) #control() only resets #conn on connect-time failure — if the established control connection later dies (server idle timeout is common in FTP), every subsequent call fails on the dead connection until the caller makes a new client. Resetting #conn when a command fails with a transport-level error would make long-lived clients self-heal.
REST is sent before EPSV/PASV. RFC 3659 §5.3 says REST must be the command immediately preceding the transfer command. vsftpd and proftpd happen to keep the restart offset across an intervening EPSV/PASV, so this works against the named targets (and your own server), but stricter servers are allowed to reset it. Moving the REST exchange to after openDataUnlocked() (right before RETR) is a one-line reorder that removes the interop risk.
550 is mapped unconditionally to not-found, but servers also use 550 for permission errors (vsftpd: "550 Permission denied." on DELE/STOR). A `/(permission
A non-VFSError thrown by a VFS implementation (or a handler bug) propagates out of run() and kills the whole control connection instead of producing a reply. Catching the generic case with a 451 Requested action aborted: local error keeps one bad operation from dropping the session; onError can still be notified.
type (and prot) are tracked in session state but never read: TYPE A is accepted and then the transfer is sent as binary, and PROT C on an implicit-TLS server still serves TLS data connections. Both are fine behaviors in practice (modern clients use TYPE I / PROT P), but accepting-and-ignoring is the worst of both options for ASCII mode — either honor TYPE A (CRLF translation on RETR/STOR) or reply 504 so the client knows. If you keep them as accepted-but-ignored, dropping the dead state fields would at least make that explicit.
Autonomous review (Claude Fable 5). This is a well-put-together package: the control-channel mutex design (lock held across verb+data, released when the returned stream settles) is clean, CRLF injection is blocked in the codec, resolvePath prevents .. escapes on the server, and the 150-before-dial dance is correctly implemented on both sides. No blocking correctness bugs found; comments below are interop robustness and small hardening items.
Failed connect is cached forever. If #doConnect() rejects (transient dial failure, wrong password once, server briefly down), #connecting keeps holding the rejected promise and #tree stays unset, so every subsequent operation on this client rejects with the same stale error until disconnect() is called manually. Clear #connecting when the promise rejects, e.g. this.#connecting = this.#doConnect().catch((e) => { this.#connecting = undefined; throw e }).
Minor: if a write fails mid-stream, the source stream is left locked and un-cancelled (the finally only closes the SMB handle). Consider reader.cancel()/releaseLock() on the error path so the producer is told to stop.
parseFileIdBothDirInfo throws RangeError on an empty buffer (the first r.u32() reads past the end). Tree.queryDirectory checks batch.length === 0 as a loop exit, which suggests a zero-length success buffer is considered possible — if so, guard with if (buffer.length === 0) return [].
Autonomous review (Claude Fable 5). Solid, readable implementation — the crypto (CMAC subkey generation, SP800-108 KDF, NTLMv2 + MIC ordering) and the wire codecs all check out against the specs, and the preauth-integrity ingest/derive ordering in #doRequest is correct. Findings below: one real availability bug, one error-mapping inconsistency, one security gap worth an explicit decision, plus minor notes.
Connect-time SMB errors bypass the VFSError mapping. #ensure() is awaited outside the try, so Smb2Errors thrown during connect — LOGON_FAILURE from session setup, BAD_NETWORK_NAME from tree connect — escape as raw Smb2Error instead of going through statusToVFSError. The BAD_NETWORK_NAME/LOGON_FAILURE/ACCOUNT_DISABLED entries in the status map in status.ts are effectively dead code today. Either move #ensure() inside the try, or map in #doConnect.
Response signatures are never verified. Requests are signed, but the signature on received messages is never checked, so an on-path attacker can tamper with or inject responses even on a "signed" session — which defeats most of the point of mandatory signing. [MS-SMB2] §3.2.5.1.3 requires the client to verify. If this is deliberately deferred like encryption, it deserves the same explicit call-out in the PR description / a TODO; otherwise verification is cheap to add here (recompute over the message with the signature field zeroed, compare constant-time-ish).
