Fix six findings from the 0.7.0 API/lifecycle review - #77
Merged
Conversation
A fresh adversarial review of the un-hammered surfaces (public API, worker lifecycle, Go non-transport) surfaced these; all verified against the code. 1. LocalAPI freeze (high): closeAllServePublications made unbounded context.Background() calls while holding the global mu on the down/logout/start path, so a wedged tailscaled froze the whole binding. Bound every LocalAPI call via boundedCallCtx(0) (30s) — #71's invariant had only covered dial/ping; 9 wrappers were still unbounded. 2. Funnel path prefix (med): a non-root funnel mount didn't strip its prefix, so funnel.forward(path:'/api') sent the backend /api/foo not /foo. Added funnelMountHandler (http.StripPrefix), matching Serve/tailscaled. 3. HTTP client fd leak (med): in send(), request.finalize() sat outside the try/catch; a throw (re-sent/custom request) leaked the request fd (no finalizer) and pinned the Go goroutine. Isolated finalize() so a failure releases both fds. 4. lastSeen garbage (med): a peer's zero LastSeen ('0001-01-01T00:00:00Z') parsed to a year-1 DateTime instead of the documented null. _parseTime now treats the zero-time sentinel as null. 5. Sync-throw (med): nodes()/whois()/useById() threw synchronously instead of rejecting the returned Future, so .catchError missed them. Made async. 6. suggest() throws vs null (med): exitNode.suggest() threw for the transient 'not ready yet' errors (ErrNoPreferredDERP/ErrNoNetMap) common right after up(). Mapped those to a null suggestion (matched by message, like isNotFound, since they cross the LocalAPI boundary). Also corrected the funnel.clear() doc (it doesn't touch Serve). Discriminating tests for 2/4/5/6 (funnel strip, lastSeen null, async rejection, transient matcher). #1 (30s timeout) and #3 (needs a live native node) aren't practically unit-testable and rely on the shared boundedCallCtx helper + review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… test-wiring gaps The adversarial review of the fixes found them correct but surfaced two completeness gaps: - Context sweep was incomplete: DuneStatus/DunePeers (go/lib.go) still made unbounded context.Background() LocalAPI calls — the same one-shot pattern fixed in localapi.go. Bounded both; a full-tree sweep now confirms every remaining context.Background() is a WithTimeout/WithCancel base. The 'no unbounded native call' invariant is complete. - The funnel and suggest regression tests only exercised the helper, not the production wiring — reverting the wiring line left them green. Extracted newFunnelTarget and exitNodeSuggestResult (the exact constructors the exports use) and pointed the tests at them, so a wiring regression now fails. Verified: reverting the funnel strip fails TestFunnelTargetStrips. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
A fresh adversarial review of the surfaces the transport audit didn't hammer (public API, worker lifecycle, Go non-transport) surfaced these. Every finding was verified against the code before fixing.
closeAllServePublicationsmade unboundedcontext.Background()LocalAPI calls while holding the globalmuon the down/logout/start path → a wedged tailscaled froze the whole binding. Bounded every LocalAPI call viaboundedCallCtx(0). #71's invariant had only covered dial/ping — 9 wrappers were still unbounded.funnel.forward(path:'/api')sent the backend/api/foonot/foo. AddedfunnelMountHandler(http.StripPrefix), matching Serve/tailscaled.send()—request.finalize()sat outside the try/catch; a throw (re-sent/custom request) leaked the request fd (no finalizer) + pinned the Go goroutine. Isolatedfinalize().LastSeenparsed to a year-1DateTimeinstead of the documentednull._parseTimenow maps the zero-time sentinel to null.nodes()/whois()/useById()threw synchronously instead of rejecting the Future (.catchErrormissed them). Madeasync.exitNode.suggest()threw for the transient "not ready yet" errors (common right afterup()) instead of returning null. MappedErrNoPreferredDERP/ErrNoNetMap(by message, likeisNotFound) to a null suggestion.Also corrected the
funnel.clear()doc (it doesn't touch Serve).Tests
Discriminating regression tests for #2 (funnel strip), #4 (lastSeen null), #5 (async rejection), #6 (transient matcher). #1 (30s deadline) and #3 (needs a live native node) aren't practically unit-testable and rely on the shared
boundedCallCtxhelper + review.Verification
Darwin
go vet+-raceclean;dart analyzeclean; unit + fd/ffi integration+117. Linux container + Headscale e2e running. Version bump to 0.7.1 + changelog to follow in this PR after review.🤖 Generated with Claude Code