0.8 cleanup: fd-cleanup unification, LocalAPI error codes, API consistency - #80
Merged
Conversation
…ubstrings
- DuneStatus/DunePeers returned post-call LocalAPI failures via jsonError
instead of localAPIError, so they emitted no code/statusCode field and a
classifiable 403/404 read back as TailscaleErrorCode.unknown on the Dart
side. Route both through localAPIError like every other wrapper.
- Removed two dead classifyLocalAPIError substrings ('no funnel attribute' /
'does not have funnel attribute') — the real upstream funnel errors both
begin 'Funnel not available; ...' and already classify via 'not available'.
- Replaced the classifier test's fabricated strings with the exact upstream
NodeCanFunnel/CheckFunnelAccess text so it catches drift on the live path.
(Considered a generic withBoundedLocalClient chokepoint for the LocalAPI
wrappers but skipped it: the per-site rendering is genuinely bespoke, all
contexts are already bounded since 0.7.1, so a generic wrapper is a poor fit
for marginal enforcement value against real churn.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion test send() tracked fd ownership across 3 separate try/catch blocks by code position, so a future inserted step could silently leak a fd (and pin the Go request goroutine). Extracted the fd-owning core into _sendOverFds with a single cleanup path: ownership is tracked by a requestFdTransferred flag, and one catch closes whatever is still owned — mirroring the cohesive inbound accept path in api/http.dart. - Documented PosixFdTransport.adopt's failure-ownership contract on adopt itself (registration failure closes the fd; only the up-front arg checks leave it untouched) — previously stated only at call sites. - Added test/integration/fd/http_send_cleanup_test.dart: injects a throwing finalize() and asserts BOTH fds are released (peers observe EOF). Verified discriminating — reverting the raw-fd close fails it with 'request fd leaked'. This closes the fault-injection coverage gap the review flagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…alidators - UDP caller-argument validation (invalid remote port/address, oversize payload) now throws ArgumentError/RangeError like serve/funnel/http, instead of TailscaleUdpException. Wire-parsing errors in _decodeDatagramEnvelope (bytes from the native side) stay TailscaleUdpException — those are operational, not caller mistakes. BEHAVIOR CHANGE: a bad argument to udp.send now surfaces as ArgumentError/RangeError. - Extracted the byte-identical serve/funnel mount validators (port/address/ path) into lib/src/api/serve_validation.dart — the rules existed in three places (serve.dart, funnel.dart, and Go); now one Dart copy. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- CI ran 'go test' without -race even though the teardown/reactor harness was written to stress register-vs-teardown under the detector. Add -race. - pr-readiness.md still listed the shared-fd-reactor as a 1.0 launch blocker; the sharded reactor shipped. Marked done. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Targeted cleanup from the three-way architecture-health review (task #33). The review found the codebase healthy (2/5 debt) with a short list of real-but-cheap items — this is that list, minus the ones inspection showed weren't worth it (see Skipped).
Genuine (close a bug-class / fix a live defect)
send()fd cleanup into one path (_sendOverFds, ownership tracked by a flag not by code position — mirrors the cohesive inbound accept path). DocumentsPosixFdTransport.adopt's failure-ownership contract onadoptitself. New fault-injection test (http_send_cleanup_test.dart) proves both fds are released whenfinalize()throws — verified discriminating.DuneStatus/DunePeersnow emit error codes: they returned post-call LocalAPI failures viajsonErrorinstead oflocalAPIError, so a classifiable 403/404 read back asTailscaleErrorCode.unknownon the Dart side. Routed throughlocalAPIError.Tidy-ups
ArgumentError/RangeErrorlike serve/funnel/http, notTailscaleUdpException. Wire-parsing errors (bytes from the native side) stayTailscaleUdpException.serve.dart,funnel.dart, and Go) now shareserve_validation.dart.classifyLocalAPIErrorsubstrings; the classifier test now uses the real upstreamNodeCanFunnel/CheckFunnelAccessstrings so it catches drift.-race; marked the shipped shared-reactor item done inpr-readiness.md.Skipped (inspection showed poor value/risk) — with reasons
withBoundedLocalClientchokepoint: per-site rendering is genuinely bespoke (pre-formatted strings, nested maps, not-found early-returns), and all contexts are already bounded since 0.7.1 — a generic wrapper is a poor fit for marginal enforcement value against real churn. The one live drift it would've prevented (DuneStatus/DunePeers) is fixed directly above.localPort ?? 0→ require-field: Go always sends these fields (verified), so it's a theoretical drift-mask, not a live bug; converting silent-0 to a throw across 7 sites in different error-handling contexts risks turning an impossible case into a stream-breaking throw.UnimplementedErroris anError— a synchronous throw is the correct loud "not implemented" signal for a stub; you don't want.catchErrorswallowing it.Verification
Darwin
go vet+-race,dart analyzeclean, unit + fd/ffi+118. Linux container + Headscale e2e running. This is 0.8 work — no version bump; changelog written when 0.8 is cut.🤖 Generated with Claude Code