fix(server): accept per-ledger SSE subscriptions on GET /events

[aaj3f]

GET /v1/fluree/events?ledger=<alias> has never worked. It answers 400 for every value — invalid type: string "…", expected a sequence — so only ?all=true has ever opened a stream.

The mechanism: EventsQuery.ledgers is a Vec<String> fed by repeated keys (?ledger=a&ledger=b), and the handler took it through axum::extract::Query, which deserializes with serde_urlencoded — and serde_urlencoded cannot build a sequence from repeated keys. It rejects the whole request before the handler runs. ?all=true survived only because it's a plain bool.

Consequence, and the reason I'd like eyes on this beyond the diff: any peer subscribed to specific ledgers rather than subscribe_all has been receiving no events at all. The native peer builds exactly this form — peer/subscription.rs pushes ledger={} per alias — and the module docs advertise ?ledger=a&ledger=b as the API. It's worth someone checking whether a deployed peer is affected. Two corrections to an earlier draft of this paragraph, both from review and both verified: this is more severe than "an unusual opt-in" — peer_subscribe_all defaults to false (config.rs:867) and validation requires either --peer-subscribe-all or an explicit --peer-ledger list (config.rs:1163-1171), so the broken form is one of only two valid peer configurations. And it is not silent — a 400 is not is_fatal() (only 401/403 are, subscription.rs:406-414), so an affected peer logs WARN "Peer SSE subscription failed, will reconnect" on every backoff cycle and loops forever. Noisy but ineffective is a more accurate description than event-less-without-notice, and it changes how someone would have found this.

Why it survived this long: every existing test constructs an EventsQuery in process, so nothing ever exercised the query string over the wire, and the peer test configs all set peer_subscribe_all: true — the one shape that happens to work.

The fix takes RawQuery and parses the pair list directly, percent/+-decoding both halves of each pair. An earlier draft of this description claimed it folded a serde_urlencoded pair list and kept that crate's decoding; it does not, and review rightly caught the mismatch. serde_urlencoded::from_str::<Vec<(String, String)>> — the pair-list shape rather than the struct shape — was then measured as the replacement, and it is not usable here: both "all" and "all=" deserialize to ("all", ""), collapsing the bare-flag form this endpoint distinguishes, and it does not reject undecodable input — it passes %ZZ through raw and replaces %FF lossily, which is the exact silent failure this PR exists to remove. It does handle repeats and key decoding correctly; two of three was not enough to adopt it. Repeated ledger= / graph-source= keys accumulate, unknown keys are ignored for forward compatibility, and a malformed query string still 400s exactly as the derived extractor did. The Deserialize derive stays for the in-process constructions and existing tests.

Covered at both levels: parser unit tests in the module (single alias, repeats, percent-encoded :, the all spellings, unknown-key tolerance), and test_events_accepts_per_ledger_subscription_over_http in proxy_integration.rs, which goes over HTTP — the gap that let this ship — across the single-alias, percent-encoded, multi-alias, mixed graph-source, all=true, and bare forms. It is mutation-verified: reverting events.rs to main while keeping the test fails it at GET /v1/fluree/events?ledger=sse:main.

Found while building an end-to-end browser-peer smoke against a real server (part of the wasm work in #1714/#1715) — the browser peer builds the same URL, so it hit the same 400. Pulled out here as a standalone fix against main because nothing about it is wasm-specific and it shouldn't wait behind that stack.

Gates: proxy_integration 30/30, server lib 154/154 at that commit, cargo clippy -p fluree-db-server --all-targets -D warnings clean, cargo fmt --check clean, workspace cargo check --all-targets clean.

A second silent failure in the same parser

Reviewing this fix's neighbourhood turned up the sibling of the same bug, so it ships here rather than separately: all accepted only the exact strings true and 1, and everything else silently meant false. ?all=TRUE, ?all=yes, ?all=on, and every typo therefore opened a 200 SSE stream that matched nothing and emitted nothing, forever, with no error for a client to notice — the identical failure shape to the 400 above, one parameter over, and one the derived extractor used to catch (a non-boolean 400'd).

all is now the one value this parser rejects rather than guesses at, because it is the subscription request: true/1/yes/on and false/0/no/off case-insensitively, bare ?all as the flag form, and anything else — empty included — a 400 naming the parameter. Deliberately not extended to ledger=: an empty alias there is one of however many were asked for, so dropping it narrows the scope without erasing it, and an existing test already pins that it is ignorable.

Gates after all three fixes: server lib 156/156 (154 on the branch before this PR's two new parser tests), proxy_integration 30/30, clippy -D warnings clean, cargo fmt --check clean, workspace check clean.

Third fix: an undecodable parameter is rejected, not used verbatim

Review found the decode fallback passed raw text through when percent-decoding failed, so ledger=books%ZZmain became the alias "books%ZZmain" — which cannot match any ledger, and so opened a 200 SSE stream emitting nothing, forever. That is the same silent-freeze shape as the two fixes above, a third time.

Two things surfaced while fixing it that are worth stating, because both make the change larger than the report:

• urlencoding::decode only fails on invalid UTF-8. A malformed escape (%ZZ, a trailing %) is left in the string and returns Ok, so routing the error case to a 400 was not sufficient on its own — the escapes are now validated before decoding. A test written against the reported case caught this on the first attempt, which is the reason it is not still latent.
• Keys were matched raw, so %6Cedger=books%3Amain — a legitimate subscription request — matched no arm and was dropped, serving the same empty stream. Keys are now decoded before matching, and an undecodable key is a 400 rather than an ignored unknown key, because the server cannot tell whether it was a ledger= it is discarding.

The fallback also returned the pre-+-substitution text, so a decode failure silently changed space handling as well; moot now that it is an error.

Operator note

Affected peers have been in a permanent connect → 400 → backoff loop, so connection churn drops when this lands. In exchange they begin receiving events for the first time and doing what events trigger — ledger preloads (subscription.rs:254) and cached-ledger refreshes (:306). A peer that has been event-less since deployment will begin syncing the moment this merges, which is the intended behavior but is not a no-op on a running fleet.

[bplatz]

Approving — the bug is real and the fix works. Two things I'd like tightened before merge, both in the parser; details inline.

Verified. Reverting events.rs to main with the test kept:

test_events_accepts_per_ledger_subscription_over_http ... FAILED
  GET /v1/fluree/events?ledger=sse:main   left: 400

It's worse than the description in one respect. peer_subscribe_all defaults to false (config.rs:867) and validation requires either --peer-subscribe-all or an explicit --peer-ledger list (config.rs:1163-1171). So the broken form is one of only two valid peer configurations, not an unusual opt-in.

And better in another — worth correcting, because it changes how someone would have found this. A 400 isn't is_fatal() (only 401/403 are, subscription.rs:406-414), so an affected peer logs WARN "Peer SSE subscription failed, will reconnect" on every backoff cycle and loops forever. That's noisy, not silent — "erroring in a way anyone would notice" is closer to what actually happens than the description allows.

Perf: nothing in the code (one parse per SSE connection), but there's an operator note worth adding. Affected peers have been in a permanent connect → 400 → backoff loop, so connection churn drops after this. In exchange they start receiving events for the first time and doing what events trigger — ledger preloads (subscription.rs:254), cached-ledger refreshes (:306). A peer that has been event-less since deployment will begin syncing the moment this lands.

Two smaller items:

• The gate tallies contradict each other — "server lib 156/156" in the first section, "server lib 154/154" after both fixes. Adding tests shouldn't lower the count; I measured 154 on the branch, so the 156 looks stale.
• The red CI is not this PR. it_ledger_lifecycle::ledger_exists_on_file_storage fails on main too (run 33133302931, fe3c198c) and on #1727 — three runs across main and two branches. It wants an issue rather than a re-run; it's blocking at least two PRs right now.

[bplatz]

The description says this "folds a serde_urlencoded pair list by hand — serde_urlencoded deserializes a pair list faithfully, repeats included, so this keeps its percent/+ decoding instead of reimplementing URL decoding."

The code doesn't do that. It splits on &/= by hand and calls urlencoding::decode, which is reimplementing URL decoding — the thing the rationale says it avoids. serde_urlencoded is already a dependency (fluree-db-server/Cargo.toml:97), and the described approach does work:

serde_urlencoded::from_str::<Vec<(String,String)>>("ledger=books%3Amain&ledger=x+y")
  => Ok([("ledger","books:main"), ("ledger","x y")])

Three lines instead of thirty, repeats and + and percent escapes handled uniformly, and it decodes keys too — the hand-rolled version matches on the raw key, so %6Cedger=a%3Ab parses to ledgers=[] and is silently dropped.

Adopting what the description already claims would resolve this, the decode-fallback issue below, and the key-decoding gap in one edit.

[bplatz]

This fallback reintroduces the exact failure the ?all= commit exists to remove. On a decode error it passes the raw text through as an alias:

input	result
ledger=books%ZZmain	Ok(["books%ZZmain"])
ledger=a%FFb	Ok(["a%FFb"])

Neither can ever match a ledger, so the client gets a 200 SSE stream that matches nothing and emits nothing, forever, with no error to notice — the identical silent-freeze shape, one parameter over.

The stated reason for tolerating a bad ledger= is that "an empty alias there is one of however many were asked for, so dropping it narrows the scope without erasing it." That holds for an empty value. An undecodable one isn't narrowing the request, it's a request the server couldn't read — and if it's the only ledger=, the subscription matches nothing at all.

Minor, same line: the fallback returns v.to_string() (the raw pair value), not the +-replaced one, so a decode failure silently changes + handling as well.

[aaj3f]

Thanks — all three are fixed, and the second one turned out to have a third defect hiding behind it.

On replacing the parser with serde_urlencoded::from_str::<Vec<(String, String)>>. You were right that the description claimed something the code doesn't do, and that mismatch is fixed. I measured the pair-list shape before adopting it, though, and it resolves one of the three problems rather than all three:

	pair-list shape
repeats + key decoding	yes — %6Cedger=a%3Ab → ("ledger", "a:b")
bare flag	no — both "all" and "all=" deserialize to ("all", "")
undecodable input	no — ledger=books%ZZmain → "books%ZZmain" raw; a%FFb → a\u{FFFD}b lossy

The second row collapses the two spellings this endpoint deliberately distinguishes (?all means true, ?all= is a 400). The third is the one that matters most here: it does not reject the input you flagged below, it passes it through exactly as the old fallback did — so adopting it would have left that bug in place while looking like a fix. The parser stays hand-rolled and the defects are fixed directly; the doc comment now records the measurement so nobody re-litigates it.

The decode fallback. Fixed — an undecodable component is a 400 rather than an alias that can never match. One thing worth flagging because it made the fix larger than the report: urlencoding::decode only fails on invalid UTF-8. A malformed escape (%ZZ, a trailing %) is left in the string and returns Ok, so routing the error case to a 400 was not sufficient on its own — the escapes are now validated before decoding. I wrote the test against your books%ZZmain case first and it caught this on the first run, which is the only reason it isn't still latent.

Keys too. Adopted your point: keys are now decoded before matching, and an undecodable key is a 400 rather than an ignored unknown key, since the server cannot tell whether it was a ledger= it is discarding. The old fallback also returned the pre-+-substitution text, so a decode failure silently changed space handling as well; moot now that it errors.

The tallies. You were right — 154 was correct on the branch and the 156 was stale. It is now genuinely 156, because this change adds two parser tests.

The red CI. Agreed, and confirmed independently: main itself was red at fe3c198c8 with the identical ledger_exists_on_file_storage failure, green at fd78564ca. Root cause was #1716 redefining ledger_exists so a retracted record answers false, against a test still asserting the older "is there a record" contract. It has since been fixed on main and main is green again, so this PR's checks should come back clean on a re-run.