From 0776dfbea4876a1664a169eb050a3f2f35a01755 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 21:23:59 +0200 Subject: [PATCH] docs: explain what someguy does not do, and why The docs covered how the routing and caching layers work, but not the choices behind them, so the reasoning lived in review threads. Someone reading only the code cannot tell a deliberate trade-off from an oversight, and two of these have already been broken by changes that looked harmless. - new response-streaming.md: the NDJSON contract, the timeout budget, and the two traps that silently defeat streaming - peer-address-caching.md gains a trade-offs section covering address-less records, detached lookups, the uncapped /peers path, and the absence of persistence - both frame the browser client as the constraint the API is tuned for - AGENTS.md leads with the contracts that must not break - cap and timeout values now reference their env vars rather than restating defaults that drift --- AGENTS.md | 40 +++++++++- README.md | 3 +- docs/environment-variables.md | 2 +- docs/metrics.md | 5 +- docs/peer-address-caching.md | 90 +++++++++++++++++++++-- docs/response-streaming.md | 133 ++++++++++++++++++++++++++++++++++ server.go | 15 ++-- 7 files changed, 269 insertions(+), 19 deletions(-) create mode 100644 docs/response-streaming.md 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)) }