Review: server ingest/query correctness + API consistency

[awksedgreep]

Summary

Correctness and API-consistency bugs in the servers that are individually small but collectively erode trust in ingest/query results.

Locations

1. Jaeger prefix-integer parsing truncates garbage: traces query.rs:801-811 parse_integer_prefix("17junk")→17 (pinned by test at :866); overflow on start/end degrades to unbounded (None) instead of 400 — fail-open to a wider scan. Dashboard since/until correctly use exact parse (:787-795).
2. Logs ingest fabricates data: logs api.rs:1670-1699 unparseable time falls back to now(); unknown level canonicalizes to "info" (:1657-1668). A typo'd column silently shifts rows instead of counting an error. parse_query_time (:1702-1712) is correctly strict-ish on the query side.
3. authctl parse_ttl overflow: timeless-authctl/src/lib.rs:313-330 quantity * 86_400 on i64 wraps (release) / panics (debug). Use checked_mul.
4. authctl policy init audience drift: lib.rs:143 emits timeless-{signal} while verifier fixtures use timeless-data-plane (auth.rs:1437-1438). Works (mint copies from policy) but cross-implementation tokens and docs disagree — pin one.
5. kid comment lies: authctl lib.rs:44-61 says "first 16 hex of SHA-256", code takes first 8 raw pubkey bytes. Fix comment or hash.
6. policy add-subject overwrites silently + accepts arbitrary scopes (lib.rs:171-189); verifier rejects at use time (fail-closed) but CLI gives no feedback.
7. Clock-skew asymmetry: auth.rs:677-681 gives +30 s leeway to nbf/iat but none to exp — 1 s-skewed clients see spurious expired_token.
8. enforce_query_limit heuristic (auth.rs:567-572): non-UTF8 skips; form-field scan false-positives on ingest bodies while {"limit":…} JSON bodies aren't caught. Verify every JSON query path sets RESULT_ROWS_HEADER so the post-check (:456-467) fires.
9. Tail ordering differs: traces publishes after durable accept (traces api.rs:461-469, deliberate); logs publishes before writer send (logs storage.rs:1211-1214) — a logs subscriber can see entries a search never returns.
10. Backup blocks the single writer: logs storage.rs:1690-1704 (flush + up to 1M optimize steps + checkpoint with 5 s retry) runs synchronously; repeated POST /api/v1/backup (open by default) is a throughput DoS. Same shape in metrics/traces.
11. No shutdown deadline: storage.rs:1579-1613 sequencing vs hung checkpoint_wal (lib.rs:58-79) can stall past orchestrator kill grace.
12. API inconsistency: flush GET vs POST varies per signal; error envelopes vary ({"error"} vs {"status":"error"} vs Prometheus shape); RESULT_ROWS_HEADER stripped on authed routes but emitted on open ones (auth.rs:456-467); no Retry-After/RateLimit-* on 429/503; logs field_values limit dance (api.rs:352-359) is obscure.

Also: unbounded regex compile per request (metrics query.rs:149-166, logs LogPredicate::Regex) with no pattern-length cap; PromQL parse input bounded only by the 10 MiB body cap with no depth cap; OTLP/log ingest admission bounded by batch count not spans/bytes (traces storage.rs:590-645, logs storage.rs:1193-1217).

Suggested fix

Exact integer parsing + bounded limits; strict ingest time/level handling; checked_mul TTL; one audience value; symmetric clock leeway; one tail contract; backup off the writer or rate-limited; shutdown deadline; one error envelope ({"error","reason"} + Prometheus shape on Prometheus routes); POST-only flush.

Source

Full code review @ bacdf6f, servers track.

[awksedgreep]

Shutdown deadline landed (16af776 on main): the whole post-signal drain (serve stop, maintenance teardown, storage flush/checkpoint/join) runs under a shared 30s ceiling in api-common (shutdown_with_deadline). On expiry the drain is aborted, the exit is logged loudly, and buffered tail data is lost exactly as on SIGKILL — which the durability contract already accepts. This also fixes a latent hang: an open live-tail stream blocked axum's graceful shutdown indefinitely. Remaining in this issue: backup off the writer thread, error-envelope unification, enforce_query_limit content-type awareness.

[awksedgreep]

Query-limit pre-check scoped to form bodies (see main commit 'auth: scope the query-limit body pre-check'): NDJSON/JSON/text ingest payloads mentioning limit= no longer 422 spuriously; POSTed query forms keep the pre-check, and actual rows remain enforced by the verified result-row header. Remaining here: backup off the writer thread, error-envelope unification.

[awksedgreep]

Item 7 (clock-skew asymmetry) fixed in 4bc014e.

exp now carries the same bounded CLOCK_SKEW_SECONDS allowance that nbf, iat and not_before already had. The asymmetry was one-directional by construction: a minting clock running fast puts claims in the verifier's future and was tolerated; one running slow puts exp in its past and was not.

The principle is now written into the code — leeway applies to claims, not to policy fields. exp/iat/nbf are stamped by the minting side, so there is a second clock to reconcile. key.not_before, key.expires_at and key.revoked come from the policy file this verifier reads, so there is no second clock, and revocation in particular must take effect promptly rather than be softened by a skew allowance. That is why the fix is deliberately narrower than "add leeway to every time comparison".

The existing boundary table pinned exp = now - 1 as expired — the behaviour being fixed — so it moved past the allowance, and a new test pins the symmetry in both directions plus the fact that the allowance is bounded.

Remaining on this issue: 4 (audience drift, timeless-{signal} vs timeless-data-plane — a pin-one decision), 10 (backup still synchronous on the single writer; now auth-scoped, so the open-by-default DoS is gone, but an authorized caller can still stall ingest), 12 (error envelopes still vary; no Retry-After/RateLimit-*; logs field_values limit dance). Flush is now POST on all three signals.

One finding not in the original list: logs has no regex pattern-length cap. Metrics got MAX_REGEX_PATTERN_BYTES (1024), but the equivalent guard on LogPredicate::Regex does not appear to exist, so half of the "unbounded regex compile per request" item is still live.

[awksedgreep]

Item 10 (backup blocks the single writer) fixed in 2ed1701.

The work genuinely needs the writer — the extension's engine state lives there — so it can't move off-thread. It is now single-flight per process instead: SingleFlight in api-common hands out one RAII guard, and an overlapping POST /api/v1/backup is refused immediately with 409 Conflict (reason: "a backup is already running") rather than queued. A repeated call now costs the writer O(1) instead of O(backup). All three signals.

Two details that shaped the implementation:

• The slot is claimed before the admission lock, so a caller that would only wait behind an in-flight backup is told so at once rather than blocking.
• Release is on Drop, so an early ?, a request future cancelled by client disconnect, and a panic all free it. A manual release would wedge the endpoint until restart after a single failed backup — there is a unit test pinning release after an unwinding holder.

409 rather than 500 because this is a transient conflict the caller can retry; the generic error path would have flattened it.

Tests: SingleFlight unit tests for exclusion, reuse and unwind-release, plus a metrics storage contract that joins two backups on one task (deterministic — the slot is claimed before the first await), asserts the second is refused, and asserts the endpoint still works afterwards.

---

Separately, a pre-existing failure this surfaced (fixed in 8ab5ed4): the flush GET→POST change under item 12 left two logs tests still sending GET, so both asserted 200 against a route that now answers 405. Both are --ignored real-extension tests, which is why a normal cargo test run stayed green — exactly the green-but-vacuous gap #45 describes. Verified pre-existing on a clean tree before fixing.

Remaining on this issue: 4 (audience drift — a pin-one decision, timeless-{signal} vs timeless-data-plane), 12 (error envelopes still vary; no Retry-After/RateLimit-* headers; logs field_values limit dance), and the logs regex pattern-length cap noted above.

[awksedgreep]

Item 4 (audience drift) resolved in 91ece72 — timeless-<signal> is the convention.

Worth recording that the finding was narrower than it reads. There is no production constant named timeless-data-plane anywhere: the verifier enforces only claims.aud != policy.audience (auth.rs:731), with audience read from the policy file, so any self-consistent pair passes. Both remaining timeless-data-plane references were inside #[cfg(test)] — a matched policy/claims fixture pair — and authctl already wrote timeless-{signal}. Nothing in the shipped system disagreed with itself, and mint copying the audience out of the policy meant an authctl-managed deployment never could.

The two gaps that were real:

• The fixtures modelled a shared audience the tool never produces, so the suite never exercised the shape a real authctl deployment has.
• The convention was undocumented, so a hand-written policy or a third-party minter had nothing to conform to.

Both closed: fixtures use the per-signal form, the authctl round-trip test pins the shape where it is produced (the verifier can't pin a shape it doesn't check), and SERVER_API_REFERENCE.md states the convention — including, explicitly, that the verifier does not enforce it. The separate signal check is the primary control; a per-signal audience is defence in depth.

Deliberately not added: verifier enforcement of the shape. It would reject every existing policy using a different audience, for a property the signal check already covers.

timeless-data-plane no longer appears anywhere in the tree.

Remaining on this issue: 12 (error envelopes still vary; no Retry-After/RateLimit-* on 429/503; logs field_values limit dance) and the logs regex pattern-length cap.

[awksedgreep]

The logs regex pattern-length cap (the extra finding from the "unbounded regex compile per request" item) is fixed in 5829350.

All three user-facing LogsQL pattern entry points — the ~"..." filter, replace_regexp, and extract_regexp — bounded the compiled program with size_limit(1 << 20) but nothing bounded the pattern text. The regex crate is linear-time in the subject, so this was never a backtracking risk; the exposure is compile cost, which scales with the source and is reachable from an unauthenticated request path.

The metrics server already caps at 1 KiB and its comment says it "mirrors the LogsQL side's posture" — that was aspirational. All three LogsQL sites now share one compile_user_regex helper carrying the same cap, so a new call site cannot silently skip it; no RegexBuilder for a user pattern remains outside the helper. The rejection echoes only a 48-character prefix, since the whole point is that the pattern may be enormous and the message goes back to the client.

The test drives all three entry points, asserts the error names the limit, asserts it does not echo the pattern back, and pins that a pattern exactly at the cap still compiles — a boundary, not a ban.

---

Running the --ignored suites for this turned up an unrelated flaky test, filed as #49: session_ten_logsql_limits_cancel_errors_and_direct_sql_reuse_the_reader fails ~2 of 3 runs on a clean tree (1 ms deadline vs a fixed 500 ms polling budget for the cancellation counter). Verified pre-existing by stashing before concluding. It matters because api_e2e is one of the suites the production gate runs.

Remaining on this issue: 12 only — error envelopes still vary ({"error"} vs {"status":"error"} vs Prometheus shape), no Retry-After/RateLimit-* on 429/503, and the logs field_values limit dance.