diff --git a/AGENTS.md b/AGENTS.md index 01de66d..fb37bde 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,6 +4,43 @@ someguy is a server implementing the [Delegated Routing V1 HTTP API](https://spe It proxies requests to the Amino DHT and other delegated routing endpoints. It is a caching proxy, not a libp2p node. +## Contracts that must not break + +Breaking one of these is easy to miss. The streaming and timeout rules below +both broke in production while the test suite and a plain `curl` passed. + +**The HTTP API follows the spec.** [Delegated Routing +V1](https://specs.ipfs.tech/routing/http-routing-v1/) defines the response +shapes, status codes, and query parameters. Adding a field, header, or endpoint +it does not define needs an [IPIP](https://specs.ipfs.tech/ipips/) first. +`Cache-Control` values come from `boxo/routing/http/server`. Do not set them in +someguy. + +**NDJSON responses stream.** A record reaches the client as soon as someguy has +it. Never add middleware that buffers the response body, and never collect +records before writing them. Compression is the known trap. It withholds small +writes until it has enough bytes to choose an encoding, which turns a stream +into one batch at the end and hides the response headers too. +`TestCompressedNDJSONFlushesEachRecord` guards this. Test with `curl +--compressed`, because plain `curl` requests no compression and streams fine +either way. See [response-streaming.md](docs/response-streaming.md). + +**someguy is tuned for browsers.** A browser client has a small request budget +and a person waiting on it. someguy spends its own resources to save the +client's: it resolves addresses server side and shares the answer through its +cache. Weigh any change that pushes work back onto the client against that. See +[who this is tuned for](docs/response-streaming.md#who-this-is-tuned-for). + +**The routing timeout stays below client deadlines.** A client that gives up +first loses every record someguy resolved, and reads it as no providers. See +[the timeout budget](docs/response-streaming.md#the-timeout-budget). + +**Work that outlives a request stays bounded.** Background lookups keep running +after the client disconnects, which is what keeps the cache warm for the next +request. Anything that spawns them must cap concurrency, or a cheap request +leaves unbounded work behind. See [deliberate +trade-offs](docs/peer-address-caching.md#deliberate-trade-offs). + ## Build and test ```bash @@ -24,6 +61,7 @@ Run `gofmt` and `go vet ./...` before committing. ## Documentation - [environment-variables.md](docs/environment-variables.md): all config flags and environment variables -- [peer-address-caching.md](docs/peer-address-caching.md): how `/providers` and `/peers` cache and refresh peer addresses +- [peer-address-caching.md](docs/peer-address-caching.md): how `/providers` and `/peers` cache and refresh peer addresses, and the trade-offs behind what someguy deliberately does not do +- [response-streaming.md](docs/response-streaming.md): NDJSON streaming contract, the timeout budget, and traps that break streaming - [metrics.md](docs/metrics.md): Prometheus metrics - [tracing.md](docs/tracing.md): OpenTelemetry tracing diff --git a/README.md b/README.md index 46fd2fe..815e482 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,8 @@ See [environment-variables.md](docs/environment-variables.md) for URL formats an ## Documentation - [environment-variables.md](docs/environment-variables.md): all config flags and environment variables -- [peer-address-caching.md](docs/peer-address-caching.md): how `/providers` and `/peers` cache and refresh peer addresses +- [peer-address-caching.md](docs/peer-address-caching.md): how `/providers` and `/peers` cache and refresh peer addresses, and the trade-offs behind what someguy deliberately does not do +- [response-streaming.md](docs/response-streaming.md): NDJSON streaming contract, the timeout budget, and traps that break streaming - [metrics.md](docs/metrics.md): Prometheus metrics - [tracing.md](docs/tracing.md): OpenTelemetry tracing diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 4d94882..14f295b 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -85,7 +85,7 @@ Default: `25s` ### `SOMEGUY_RECORDS_LIMIT` -Maximum providers or peers returned per `Accept: application/json` request. [HTTP Routing v1 ยง4.1.5](https://specs.ipfs.tech/routing/http-routing-v1/) recommends `100`. Set to `0` to disable the cap. +Maximum providers or peers returned per `Accept: application/json` request. [HTTP Routing v1, section 4.1.5](https://specs.ipfs.tech/routing/http-routing-v1/) recommends `100`. Set to `0` to disable the cap. Default: `100` diff --git a/docs/metrics.md b/docs/metrics.md index 3e75147..1611739 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -29,8 +29,9 @@ When Someguy aggregates other `/routing/v1` endpoints, `boxo/routing/http/client When a provider record arrives without addresses and the cache has none, Someguy dispatches a `FindPeer` in the background. These outlive the request that -triggered them, so they are capped (512 concurrent per instance by default) and -tracked here. See [peer-address-caching.md](peer-address-caching.md). +triggered them, so they are capped per instance by +[`SOMEGUY_CACHED_ADDR_BOOK_MAX_CONCURRENT_FIND_PEERS`](environment-variables.md#someguy_cached_addr_book_max_concurrent_find_peers) +and tracked here. See [peer-address-caching.md](peer-address-caching.md). - `someguy_cached_router_find_peer_lookups_in_flight`: gauge of background lookups currently running. Compare against the cap: steady state well below it means normal traffic never reaches the limit. - `someguy_cached_router_find_peer_lookups_rejected`: counter of lookups skipped because the cap was reached. A sustained increase means providers are being dropped that Someguy would otherwise have resolved, so either raise the cap or look at where the traffic is coming from. diff --git a/docs/peer-address-caching.md b/docs/peer-address-caching.md index 12631a2..611270a 100644 --- a/docs/peer-address-caching.md +++ b/docs/peer-address-caching.md @@ -133,12 +133,14 @@ stream ends are dropped. A background `FindPeer` keeps running after the request that triggered it ends, because finishing it fills the cache for whoever asks next. That also means a -client can disconnect and leave the work behind, so these lookups are capped at -512 at a time per instance. At the cap the lookup is skipped and the record is -dropped, the same as for a peer under connect-failure backoff. Normal traffic -runs about 20 at once, well below the cap; watch -`someguy_cached_router_find_peer_lookups_rejected` in -[metrics.md](metrics.md) to confirm it stays that way. +client can disconnect and leave the work behind, so these lookups are capped +per instance by +[`SOMEGUY_CACHED_ADDR_BOOK_MAX_CONCURRENT_FIND_PEERS`](environment-variables.md#someguy_cached_addr_book_max_concurrent_find_peers). +At the cap the lookup is skipped and the record is dropped, the same as for a +peer under connect-failure backoff. The default sits more than an order of +magnitude above what normal traffic uses, so it only engages far outside it. +Watch `someguy_cached_router_find_peer_lookups_rejected` in +[metrics.md](metrics.md) to confirm that stays true. ### `/routing/v1/peers/{peerid}` @@ -168,3 +170,79 @@ addresses over such a window. Both endpoints follow this rule: they read the cache first and fall back to a DHT lookup only when the cache cannot answer. + +## Deliberate trade-offs + +Several things someguy does not do look like oversights. They are choices, and +the reasoning behind them is not visible in the code. + +### Records without addresses are dropped, not returned as bare peer IDs + +A provider record whose address lookup fails is omitted from the response. +someguy could return the peer ID alone and let the client resolve it, and the +[spec has a way to ask for that](https://specs.ipfs.tech/routing/http-routing-v1/#filter-addrs-providers-request-query-parameter): +`filter-addrs=unknown` means "keep providers whose multiaddrs are unknown". +someguy does not honor it today, because the cache fallback drops those records +before the filter layer ever sees them. + +The reason is what a client does next. Its only move is to call +`/routing/v1/peers/{peerid}`, which reaches the same someguy that just failed +to resolve that peer, and runs the same DHT walk. One response with a handful +of address-less providers becomes a handful of peer lookups, each taking +seconds. + +Which way that trade should go depends on the client. A browser page shares one +connection pool across everything it loads, and a client that fans out on every +record can saturate its own request budget and effectively DoS itself, while +someguy pays for every walk anyway. Resolving once on the server and sharing +the answer through the cache spends someguy's resources instead of the +browser's, and someguy has more of them. See [who this is tuned +for](response-streaming.md#who-this-is-tuned-for). + +Some of those peers are also withheld on purpose. A peer under connect-failure +backoff never gets a lookup dispatched at all, so passing its ID to a client +would send it after a peer someguy already knows is unreachable. + +If a client can run its own DHT queries and wants the raw IDs, wiring up +`filter-addrs=unknown` is the supported path. It needs the filters to reach the +router, which the boxo HTTP server does not pass through today. + +### A background lookup is not aborted when the client disconnects + +Closing the connection stops delivery, but the dispatched `FindPeer` keeps +running on its own context for up to `DispatchedFindPeersTimeout`. Finishing +the lookup fills the cache, so the next client asking for that peer gets an +instant answer instead of paying for the same walk. + +The cost is that a cheap request can leave expensive work behind, which is what +the concurrency cap above exists to bound. Without it, a client could open +requests, close them immediately, and leave an unbounded pile of DHT walks +running. + +### `/peers` is not capped the same way + +The cap covers background lookups only. A direct `/routing/v1/peers/{peerid}` +request runs on the request context, so it stops when the client disconnects +and cannot outlive the request. Its concurrency is bounded by how many requests +are in flight. + +Skipping a lookup also means something different on each path. For a background +lookup the record is quietly omitted, which the client cannot tell from a peer +that had no addresses. For a direct request it would mean failing a request a +client is actively waiting on. The direct path has a different weakness worth +knowing: it consults no backoff, so a repeated request for an unresolvable peer +pays a fresh DHT walk every time. + +### Nothing is persisted across restarts + +someguy passes no datastore to the DHT, so value records, including IPNS +records written through `PUT /routing/v1/ipns/{name}`, live in memory and are +lost on restart. The address cache is in memory too. + +Adding a persistent datastore is possible and small, but it buys little as +deployed. A `PUT` lands on one instance behind the load balancer, and a later +`GET` lands on any of them, so a per-instance store answers only a fraction of +reads. What actually makes a record retrievable is the DHT publish that the +`PUT` already performs. Persistence would need a datastore shared across +instances, and keeping a record alive past its expiry would need a republish +loop. Both are design work rather than configuration. diff --git a/docs/response-streaming.md b/docs/response-streaming.md new file mode 100644 index 0000000..6050a6f --- /dev/null +++ b/docs/response-streaming.md @@ -0,0 +1,133 @@ +# Response Streaming and Timeouts + +A `/routing/v1` request can take seconds. someguy sends results as it finds +them rather than waiting for all of them, so a client can start dialing the +first provider while someguy is still resolving the rest. + +This document covers the streaming contract, the timeout budget, and the two +traps that break streaming. + +## Who this is tuned for + +someguy is tuned for browser clients, and a browser works under tighter limits +than a server. A page shares one connection pool across everything it loads, so +every routing lookup is a request it cannot spend on content. Someone is waiting on the result, so a provider that arrives in two +seconds is useful and the same provider after twenty usually is not. A tab has +less memory and CPU than a server, and a service worker running alongside the +page it serves has less still. + +Two behaviors follow, and both look wrong from a server's point of view. +Results go out as someguy finds them, so a client can act on the first usable +provider instead of waiting for the slowest lookup in the batch. Providers +whose addresses cannot be resolved are left out rather than returned as bare +peer IDs, because someguy resolves them once and shares the answer through its +cache. See [deliberate +trade-offs](peer-address-caching.md#deliberate-trade-offs). + +A client that runs its own DHT and has resources to spare would want the +opposite of both. + +## Streaming only happens with NDJSON + +A client gets streaming only when it asks for it: + +| `Accept` | Behavior | +| --- | --- | +| `application/x-ndjson` | one record per line, flushed as each is found | +| `application/json` | one document, sent after every result is collected | +| missing or `*/*` | treated as `application/json` | + +The JSON path reads the whole result set into memory before it writes +anything, so it pays the full lookup latency. Send `Accept: +application/x-ndjson` if you want results early. Helia and verified-fetch do, +through `@helia/delegated-routing-v1-http-api`. + +## An open connection means "still working" + +someguy holds the response open while background address lookups are still +running. It closes the stream when it has nothing left to resolve. + +That gives a client a signal it can use without any extra protocol. While the +connection is open, more records may still arrive. Once it closes, someguy is +done. An empty NDJSON stream that closes immediately means someguy found +nothing, not that it gave up early. + +There is no "retry after" hint for streams. NDJSON sends its headers with the +first record, before someguy knows whether later lookups will succeed, so no +header can describe the outcome of a stream. The only channels left would be a +trailing metadata record or an HTTP trailer, and neither is in the [Delegated +Routing V1 spec](https://specs.ipfs.tech/routing/http-routing-v1/). + +## The timeout budget + +Two clocks run at the same time, and they must not tie. + +``` +client deadline |-------------------------------------| +someguy routing |----------------------------| + ^ ^ + someguy flushes client gives up +``` + +someguy stops its routing lookups at +[`SOMEGUY_ROUTING_TIMEOUT`](environment-variables.md#someguy_routing_timeout), +and its default is set below the deadline the reference client applies. Helia's +delegated routing client aborts the whole request after 30 seconds, and it +starts counting before someguy does, because the request has to reach someguy +first. + +If both used the same value, a slow lookup would end with the client aborting +at the same moment someguy was about to send. Every record someguy resolved +would be lost, and the client would report no providers. Keep the timeout below +the deadline your clients apply, and leave room for network latency. + +> [!IMPORTANT] +> Raising `SOMEGUY_ROUTING_TIMEOUT` to or above the client deadline does not +> return more results. It returns fewer, because the client stops listening +> first. + +## Trap: middleware that buffers small writes + +Streaming breaks if anything between the handler and the socket holds bytes +back. someguy compresses responses, and compression middleware usually waits +for a minimum body size before it decides whether to compress. Until it +decides, a flush does nothing. + +Provider records are small, often under 200 bytes. With a minimum size in +place, a provider someguy had already resolved would sit in the buffer until a +later record filled it, or until the handler returned. Response headers were +held back the same way, so the request looked hung rather than slow. + +someguy therefore compresses from the first byte (`MinSize(0)` in +`newCompressionAdapter`). `TestCompressedNDJSONFlushesEachRecord` guards this. +It asserts that the first record is readable while the handler is still +writing, and that responses are still compressed, so the test cannot be +satisfied by turning compression off. + +Note that only clients sending `Accept-Encoding: gzip` were affected. A plain +`curl` requests no compression, so it streamed correctly the whole time. Test +streaming with `curl --compressed`, or the trap stays invisible. + +## Trap: a client that fans out on every record + +someguy returns providers as it finds them, so results arrive early and in +pieces. A client that starts a fresh `/routing/v1/peers/{peerid}` request for +every record it sees can issue dozens of lookups for a single content request. + +Nothing in the browser stops this. The six-connection limit applies to +HTTP/1.1 only. Over HTTP/2 the cap is whatever the server advertises, and +`delegated-ipfs.dev` advertises 100 concurrent streams. Requests from a +service worker have no tab attached, so Chrome's per-tab request scheduler +does not throttle them at all. + +Each of those lookups costs someguy a DHT walk, and a failed walk caches +nothing, so the next client repeats it. Bound the concurrency client side and +prefer the addresses someguy already returned. + +## Related + +- [peer-address-caching.md](peer-address-caching.md): where the addresses in a + response come from, and the trade-offs behind them +- [environment-variables.md](environment-variables.md): `SOMEGUY_ROUTING_TIMEOUT` + and the rest of the configuration +- [metrics.md](metrics.md): what to watch in production diff --git a/server.go b/server.go index c3ab9f4..d2fb552 100644 --- a/server.go +++ b/server.go @@ -78,14 +78,13 @@ func init() { // newCompressionAdapter builds the response compression middleware. // -// MinSize(0) is load-bearing, not a tuning knob. The middleware defers the -// compress-or-not decision until it has buffered MinSize bytes, and until it -// decides, Flush is a no-op. NDJSON records are routinely smaller than the -// default 200 bytes, so a record that is ready to send sits in that buffer -// until a later record fills it or the handler returns. That turns -// /routing/v1 streaming into a single batch delivered at the end, which is -// exactly what a client waiting on early results cannot use. Compressing -// from the first byte keeps the flush after every record meaningful. +// Do not raise MinSize. The middleware defers the compress-or-not decision +// until it has buffered MinSize bytes, and until it decides, Flush is a no-op. +// NDJSON records are routinely smaller than the default 200 bytes, so a record +// that is ready to send sits in that buffer until a later record fills it or +// the handler returns. That turns /routing/v1 streaming into a single batch +// delivered at the end, which a client waiting on early results cannot use. +// Compressing from the first byte keeps the flush after every record working. func newCompressionAdapter() (func(http.Handler) http.Handler, error) { return httpcompression.DefaultAdapter(httpcompression.MinSize(0)) }