Conversation
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The parser-aware seam paths are well covered, but the new RSC streaming implementation introduces CPU-scaling, false-positive matching, and chunk-dependent fallback behavior that should be corrected before merging.
[P3] Update the deprecation guidance to name an available replacement (crates/trusted-server-core/src/integrations/nextjs/html_post_process.rs:98)
The public functions are still re-exported for compatibility, but their documentation and compiler deprecation notes tell callers to use NextJsHtmlPostProcessor, which this PR deletes.
Please point callers to the enabled Next.js streaming integration, or state that these compatibility functions have no direct public replacement.
| .iter() | ||
| .map(|payload| payload.original.as_str()) | ||
| .collect(); | ||
| match classify_rsc_group(&payloads, self.limit) { |
There was a problem hiding this comment.
[P2] Classify unresolved RSC groups incrementally
resolve_group calls classify_rsc_group after every payload, and that function rebuilds and rescans the entire logical group. At the allowed maximum of 256 payloads, an 8.96 MiB T-chunk causes roughly 1.15 GiB of cumulative copying and scanning.
A native debug probe with fixed 8.96 MiB content took 870 ms for 32 segments and 4,510 ms for 256 segments. Large but permitted responses can therefore exhaust an edge worker’s CPU budget before releasing output.
Please retain classifier state and feed each new payload once, or enforce a cumulative-work limit that restores the group before repeated scans become expensive.
| pub(crate) static RSC_PUSH_CALL_PATTERN: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r#"(?s)(?:(?:self|window)\.__next_f\.push|\(\s*(?:self|window)\.__next_f\s*=\s*(?:self|window)\.__next_f\s*\|\|\s*\[\]\s*\)\s*\.push)\(\[\s*1\s*,\s*(['"])"#, | ||
| r#"(?s)(?:(?:(?:self|window)\.)?__next_f\.push|(?:(?:\(\s*)?(?:self|window)\.)?__next_f\s*=\s*(?:self|window)\.__next_f\s*\|\|\s*\[\]\s*\)\s*\.push)\(\[\s*1\s*,\s*(['"])"#, |
There was a problem hiding this comment.
[P2] Do not match __next_f as a suffix of arbitrary member expressions
Making the self./window. qualifier optional allows this regex to match expressions such as:
other.__next_f.push([1, "https://origin.example.com/path"])The production placeholder rewriter then treats that payload as Next.js Flight data and rewrites its URL. A probe changed the example above to use the proxy host. The base regex accepted only self.__next_f and window.__next_f.
Please keep qualified calls strict and support a streamed-away qualifier with a separate alternative anchored to the beginning of the claimed fragment. Negative tests should cover other object properties, identifier suffixes, and string/comment literals.
| } | ||
|
|
||
| let payload = &content[payload_start..payload_end]; | ||
| let exceeds_limit = payload.len() > limit |
There was a problem hiding this comment.
[P2] Do not apply the group limit across independent payloads in one parser call
captured_payload_bytes includes every placeholder queued while HtmlRewriterAdapter::process_chunk is running. The downstream processor cannot resolve and decrement an already-complete group until the entire parser call returns.
As a result, two independent payloads that are individually below max_combined_payload_bytes enter document-wide bypass when they arrive in one source chunk, but rewrite successfully when the same HTML is delivered in separate chunks. With a 100-byte limit and two 65-byte payloads, a single call produced no proxy-host rewrites, while two calls rewrote both payloads.
Please enforce the cumulative limit on the downstream unresolved semantic group rather than the parser’s entire pending queue. The parser can retain an individual-script limit while the parser-to-processor queue is bounded separately.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Parser-confirmed body seam and bounded Next.js streaming, verified by execution rather than inspection: with the Next.js processor registered a non-final chunk emits real output pre-EOF, and a false </body> literal inside RSC data now streams past while the auction is still pending. BodyCloseHoldBuffer and every auction-coordination scan for </body are gone, and all 19 CI checks pass.
Two findings block. The RSC_PUSH_CALL_PATTERN widening changes behaviour for scripts that are not Next.js RSC at all, and the T-chunk escape scanner panics on publisher-controlled input reachable through the new classification path. Neither comment carries a one-click suggestion: I applied the obvious regex narrowing in a scratch worktree and it broke html_processor_rewrites_rsc_stream_payload_with_chunked_input, so the fix is described in prose instead of proposed as bytes I could not stand behind.
Blocking
🔧 wrench
- Widened RSC push pattern rewrites unrelated publisher scripts — see inline at
crates/trusted-server-core/src/integrations/nextjs/shared.rs:21 - Escape scanner panics on non-char-boundary indices — see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc.rs:210
Non-blocking
🤔 thinking
- Deferred seam token is minted by construction but claimed by convention — see inline at
crates/trusted-server-core/src/publisher.rs:650 release_bypassrequires every captured placeholder in the current chunk — see inline atcrates/trusted-server-core/src/integrations/nextjs/rsc_stream.rs:346
♻️ refactor
- Parser-side classification passes
usize::MAXinstead of the configured bound — see inline atcrates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:114
⛏ nitpick
- Parity test shares one auction counter across adapters — see inline at
crates/trusted-server-integration-tests/tests/parity.rs:1112 - Parity test uses Axum as its own oracle — see inline at
crates/trusted-server-integration-tests/tests/parity.rs:1176
Cross-cutting / body-level findings
-
🏕
html_post_process.rsis dead surface, and the PR description overstates its removal. The description says the whole-document post-processor was removed; 231 lines went, but 674 remain.post_process_rsc_htmlandpost_process_rsc_html_in_placehave zero callers anywhere incrates/outside this module's own tests (verified by workspace grep) — roughly 150 lines of production code including a second fulllol_htmlre-parse infind_rsc_push_scripts, plus ~400 lines of test module, all compiled into the wasm build behind#[allow(deprecated)]. Their doc comments still point atNextJsHtmlPostProcessor, which this PR deletes, so the surviving documentation describes a type that no longer exists. There are no callers left to migrate, so CLAUDE.md's "migrate callers and delete legacy APIs" applies cleanly. Deleting the module and themod.rsre-export would drop ~674 lines of wasm-compiled dead weight; at minimum the stale doc comments need fixing. -
📝 The adapter regressions landed in a different crate than the plan specifies, leaving three CI gates blind to this path. The plan (
docs/superpowers/plans/2026-09-07-850-parser-aware-body-hold-nextjs-streaming.md:839-891, Task 7 Steps 5 & 7) prescribes "one buffered route regression in each adapter test module", listscrates/trusted-server-adapter-{axum,cloudflare,spin}/tests/routes.rsunder Modify, and gives per-adapter verification commands. None of those three files changed —git diff --name-onlyshows zerotests/routes.rsin this PR. The single fixture lives only incrates/trusted-server-integration-tests/tests/parity.rs. Consequence:cargo test-axum,cargo test-cloudflare, andcargo test-spin(CI gate 3) do not exercise the Next.js/auction path at all, and the newpub fn routes_with_settings_and_servicesadded to all three adapters is dead code within its own crate — its only consumer is the parity suite. The plan's completion review does disclose the relocation, but the Step 5/7 recipe and the Modify list were never corrected, so the plan still claims coverage that does not exist. Either move a fixture into each adapter's own tests, or amend the plan and note explicitly that per-adapter gates do not cover this path. -
📌 Escape-scanner panic is pre-existing on
main, not introduced here — flagged because this PR widens its reach. I verifiedEscapeSequenceIteris byte-identical at the merge base and that the same payload reached it there throughrewrite_rsc_scripts_combined_with_limit, so this is not a regression and is legitimately out of scope for a fix in this PR. It is called out because the newclassify_rsc_groupcall site runs the scanner on borrowed publisher fragments inside alol_htmlcallback, which is a materially wider and earlier exposure than the old post-processor path. A follow-up issue is the right home if you would rather not grow this PR.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test: PASS (required)
- cargo test (ts CLI, native): PASS
- vitest: PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test (axum native): PASS
- Analyze (rust): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo fmt: PASS (required)
- prepare integration artifacts: PASS
- Analyze (actions): PASS
- format-docs: PASS (required)
| pub(crate) static RSC_PUSH_CALL_PATTERN: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r#"(?s)(?:(?:self|window)\.__next_f\.push|\(\s*(?:self|window)\.__next_f\s*=\s*(?:self|window)\.__next_f\s*\|\|\s*\[\]\s*\)\s*\.push)\(\[\s*1\s*,\s*(['"])"#, | ||
| r#"(?s)(?:(?:(?:self|window)\.)?__next_f\.push|(?:(?:\(\s*)?(?:self|window)\.)?__next_f\s*=\s*(?:self|window)\.__next_f\s*\|\|\s*\[\]\s*\)\s*\.push)\(\[\s*1\s*,\s*(['"])"#, |
There was a problem hiding this comment.
🔧 wrench — This widening has no left boundary, so it now matches __next_f.push([1,"…") after any receiver, and ordinary publisher scripts get their string literals rewritten.
Verified end-to-end through the real pipeline: myAnalytics.__next_f.push([1,"https://origin.example.com/track"]) is captured into captured_payloads, replaced with __ts_rsc_<ns>_0__, and the stream processor substitutes a host-rewritten payload. Same for foo.bar.__next_f.push(...), window.myapp.__next_f.push(...), and — because the pattern also has no identifier boundary — a__next_f.push(...). On main none of these match.
Failure scenario: a publisher ships any non-Next.js script containing .__next_f.push([1,"…"]) (an analytics shim, a vendor bundle, a variable that happens to end in __next_f) → its string literal is silently URL-rewritten with no warning and no way for the publisher to opt out.
No suggestion block here, deliberately. I tried the obvious narrowing — restoring (?:self|window)\. on the first alternative — applied it in a scratch worktree and ran the gate. It fails integrations::nextjs::tests::html_processor_rewrites_rsc_stream_payload_with_chunked_input (SIGABRT under Viceroy). That test uses chunk_size = 32, which fragments the script text so claimed_start trims the self. prologue and the surviving claim genuinely begins at a bare __next_f — so the bare-receiver alternative is load-bearing, and narrowing the regex alone is wrong. I also confirmed a negative-lookbehind variant satisfies every case but does not compile: regex 1.12 rejects (?<!…) with a parse error.
Proposed fix (apply manually — belongs in the caller, not the pattern). The bare receiver is only legitimate when the claim was actually trimmed, i.e. when prior_probe is non-empty. When prior_probe is empty and claimed_start == 0, the text is a complete unfragmented script and should require the self./window. receiver. Thread that distinction into matching rather than relaxing the pattern for both cases — for example keep two compiled patterns (strict for the untrimmed case, bare-permitted for the trimmed case) and select on prior_probe.is_empty(), or check the byte before identifier_start is not in [A-Za-z0-9_.$] before accepting a bare match.
Whichever shape you pick, a regression test for myAnalytics.__next_f.push([1,"https://origin.example.com/x"]) staying byte-identical would lock this down — there is currently no test asserting a non-RSC __next_f receiver is left alone.
|
|
||
| /// Find all T-chunks in content, optionally skipping markers. | ||
| fn find_tchunks_impl(content: &str, skip_markers: bool) -> Option<Vec<TChunkInfo>> { | ||
| fn scan_tchunks_impl(content: &str, skip_markers: bool) -> TChunkScan { |
There was a problem hiding this comment.
🔧 wrench — scan_tchunks_impl is the new entry point that reaches EscapeSequenceIter, and that iterator slices &str at indices it never checks for char boundaries. Publisher-controlled RSC text can therefore panic the HTML pipeline, which under wasm32-wasip1 traps and returns 500 for the page.
Three sites in EscapeSequenceIter::next, all guarded on length only:
| Line | Slice | Input that panics |
|---|---|---|
rsc.rs:156 |
&self.str_ref[self.pos..] |
\x4é |
rsc.rs:124 |
&self.str_ref[self.pos + 2..self.pos + 6] |
\u12😀 |
rsc.rs:133 |
&self.str_ref[self.pos + 8..self.pos + 12] |
\ud83d\u12😀 |
The \x guard self.pos + 3 < self.bytes.len() bounds length but not boundaries; the \u guards slice before validating the hex.
Reproduced end-to-end from ordinary origin HTML through create_html_processor + StreamingPipeline — not just by calling the iterator directly:
<html><body><script>self.__next_f.push([1,"1:T9,\x4ézzzzzzzz"])</script></body></html>
thread '…' panicked at crates/trusted-server-core/src/integrations/nextjs/rsc.rs:156:33:
start byte index 9 is not a char boundary; it is inside 'é' (bytes 8..10) of `1:T9,\x4ézzzzzzzz`
To be clear about scope: this is pre-existing, not a regression. EscapeSequenceIter is byte-identical at the merge base, and the same payload reached it on main via rewrite_rsc_scripts_combined_with_limit. It is raised here because this PR materially widens the exposure — classify_rsc_group now runs the scanner on borrowed publisher fragments inside a lol_html text callback (rsc_placeholders.rs:114), which is earlier and more reachable than the old end-of-document post-processor, and only needs __next_f.push([1," plus an N:TX, prefix to trigger.
Proposed fix (apply manually — three separate slices outside this PR's hunks):
// \x: verify the advanced position is a char boundary before continuing.
if esc == b'x' && self.pos + 3 < self.bytes.len() {
if !self.str_ref.is_char_boundary(self.pos + 4) {
return None;
}
self.pos += 4;
return Some(EscapeElement { byte_count: 1 });
}
// \u: use get() so a non-boundary end yields None instead of panicking.
if esc == b'u' && self.pos + 5 < self.bytes.len() {
let Some(hex) = self.str_ref.get(self.pos + 2..self.pos + 6) else {
return None;
};
// … and likewise for hex2 at self.pos + 8..self.pos + 12with the same get() treatment for self.str_ref[self.pos..] at line 156. A None here should surface as TChunkScan::Invalid, which the streaming path already handles by restoring originals unchanged — the hydration-safe fallback this design already commits to.
Entirely reasonable to split this into a follow-up issue since it predates the PR; flagging it on the changed entry point so the decision is explicit rather than implicit.
| let is_html = is_html_content_type(¶ms.content_type); | ||
| let is_rsc_flight = | ||
| content_type_contains_ascii_case_insensitive(¶ms.content_type, "text/x-component"); | ||
| let inline_seam_token = deferred_inline_seam_token( |
There was a problem hiding this comment.
🤔 thinking — PublisherBodyProcessor::new mints the seam token as a side effect of construction and installs it as deferred_inline_marker, so lol_html will emit <!--ts-inline-body-close-…--> at the structural </body>. Whether that token is ever removed depends entirely on the caller remembering to call take_inline_seam_token() and wire the result into an AuctionHoldState. Only the lazy Fastly path does; process_response_streaming_async constructs the same processor and hands it to process_body_chunks_async, which has no seam controller — every processed byte goes straight to the writer.
I confirmed this is not live today: both call sites of process_response_streaming_async are reached only when dispatched_auction is None or is_html is false, so the token is None in both. No action needed for correctness right now.
The concern is that the invariant is invisible at the construction site and unenforced by types. Failure scenario for a future change: any caller that routes an HTML body with a pending auction through process_response_streaming_async ships the raw comment token to the browser and injects zero bids — silently, with no error and no telemetry signal, since the auction still collects normally at EOF.
Returning the token from the constructor rather than stashing it — let (processor, seam_token) = PublisherBodyProcessor::new(…) — would make discarding it a visible let _ = at the call site instead of an invisible omission. The design doc calls out this exact hazard ("This prevents any constructor that lacks a seam controller from producing an unresolved token"), so tightening it here would close the gap the spec anticipated.
| } | ||
| } | ||
|
|
||
| fn release_bypass(&mut self, current: &[u8]) -> io::Result<Vec<u8>> { |
There was a problem hiding this comment.
🤔 thinking — release_bypass drains every captured payload from document state and passes them all to substitute_payloads, which hard-errors when any placeholder is absent from the bytes it was given:
Err(Custom { kind: Other, error: "Next.js RSC captured placeholder is missing from held output" })
Reproduced at unit level: two payloads captured, the first classifies Invalid (setting bypass) while the second placeholder has not yet reached the output stream. The first process_chunk returns that error, and the next returns "Next.js RSC generated placeholder remained after substitution". Both propagate as io::Error and abort the response — the opposite of the byte-preserving fallback this design promises for invalid RSC data.
Being straight about reachability: I could not trigger this end-to-end. Capture and placeholder emission are atomic within a single rewrite_complete call (rsc_placeholders.rs:80-87), so the parser cannot get ahead of the output stream, and two separate e2e attempts through StreamingPipeline both completed cleanly. So this reads as defense-in-depth that is currently unreachable rather than a live bug.
Worth either a comment recording why the invariant holds (capture and emission are atomic, so a drained payload is always present in the current or held bytes), or making the drain tolerant — skip placeholders not found in the current bytes and leave them queued for a later chunk — so the fallback degrades to unchanged bytes rather than a 500 if that atomicity ever changes. The same shape appears via resolve_group's segment-limit branch at rsc_stream.rs:288-298, which sets bypass_rsc and calls release_group without draining, then relies on the loop's bypass check to call release_bypass with only the remainder of the current chunk.
| let unsafe_continuation = find_rsc_push_payload_range(content) | ||
| .map(|(start, end)| { | ||
| matches!( | ||
| classify_rsc_group(&[&content[start..end]], usize::MAX), |
There was a problem hiding this comment.
♻️ refactor — This inspection passes usize::MAX as the combined-payload bound, so the configured max_combined_payload_bytes is bypassed at exactly the point where the work happens inside a lol_html text callback. The only remaining guard is MAX_REASONABLE_TCHUNK_LENGTH (100 MB) in scan_tchunks_impl.
Failure scenario: an over-limit one-fragment script declaring 1:T5F5E100, (99 MB) with short actual content makes consume_unescaped_bytes walk the entire fragment escape-by-escape before returning NeedMore. Bounded by fragment length, so not unbounded — but it is CPU spent inside the parser callback on data the configured limit already said was too large to process.
limit is already in scope in this function, and passing it here costs nothing while making the bound uniform with the stream-side call in rsc_stream.rs:303. The classification result is unchanged for any group that is actually within bounds.
(compile-verified only — please re-run the matching cargo test-* alias after applying)
| .into_bytes() | ||
| .expect("should buffer adapter output"); | ||
| let html = String::from_utf8(body.to_vec()).expect("should emit UTF-8 HTML"); | ||
| assert_eq!( |
There was a problem hiding this comment.
⛏ nitpick — One AtomicUsize is shared across all three sequential router runs and asserted positionally as index + 1. It passes because the loop is sequential, but the assertion cannot distinguish per-adapter behaviour from the running total: if Axum dispatched two auctions and Cloudflare zero, the three checkpoints would still read 1, 2, 3 and the test would stay green — while a genuine parity violation went unreported.
client.auction_requests.store(0, Ordering::SeqCst) before each request plus assert_eq!(…, 1) makes each adapter's count independently meaningful and removes the positional coupling.
| ); | ||
| assert!( | ||
| html[bids..].ends_with("</script></body></html>"), | ||
| "{adapter} should place bid markup immediately before the body close" |
There was a problem hiding this comment.
⛏ nitpick — expected_html is seeded from whichever adapter runs first (Axum) and the other two are compared against it, so this assertion proves the three adapters agree rather than that any of them is right. The plan (Task 7, Step 5) asked for a comparison against the core expected bytes.
In practice the important bytes are pinned independently just above — the reconstructed Flight payload with its recomputed T length, script ordering, and the </script></body></html> tail — so a shared-core regression would still be caught by those. But if core output changed for all three adapters identically, this particular assert_eq! stays green. Worth a one-line comment saying that explicitly, so a future reader does not mistake it for an absolute oracle.
Address review feedback on the parser-aware body hold. Require a qualified `self.`/`window.` receiver again. The widened pattern matched `myAnalytics.__next_f.push([1,"url"])` in unrelated publisher scripts and rewrote their string literals. The bare form was load-bearing because the receiver can stream away before `__next_f` is recognised, so the receiver is now retained in a bounded context on the document state and verified out of band; an anchored pattern is used only once that context proves it. Reject a match whose preceding byte continues a member expression. Keep the escape scanner on character boundaries. `\x` advanced four bytes without checking, so publisher input such as `1:T9,\x4é` panicked the HTML pipeline and returned 500. Only the boundary is validated, not the hex digits, so the unescaped byte count that drives T-chunk length recomputation is unchanged for input that previously scanned. Classify a group incrementally. Rebuilding and rescanning the whole group per payload cost 2.4s for an 8.96MiB T-chunk over 256 payloads; feeding each payload once costs 19ms, and the cost no longer scales with segment count. Chunk content consumption and non-chunk segment inspection both resume where they stopped, and EOF reclassifies with nothing held back. Scope the captured-payload limits. `max_combined_payload_bytes` no longer bounds the parser queue in aggregate, so two independent payloads sharing a source chunk stop bypassing each other; the queue is bounded by the parser's own script-buffer budget and a payload count instead. Return the deferred inline seam token from `PublisherBodyProcessor::new` so a caller without a seam controller discards it visibly, and log when one is dropped, since a debug assertion is compiled out of the wasm builds that ship. Delete `html_post_process.rs`. It had no callers outside its own tests and documented a type this work removes. Share the Next.js auction fixture through `test-utils` so each adapter carries its own buffered regression and the parity suite stops duplicating it. Assert on the recomputed Flight `T` length rather than a substring the split fixture can never contain.
3 participants
Summary
</body>tail, preventing inline JavaScript or JSON literals from holding most of the page.Changes
crates/trusted-server-core/src/html_processor.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/mod.rscrates/trusted-server-core/src/integrations/nextjs/html_post_process.rscrates/trusted-server-core/src/integrations/nextjs/mod.rscrates/trusted-server-core/src/integrations/nextjs/rsc.rscrates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rscrates/trusted-server-core/src/integrations/nextjs/rsc_stream.rscrates/trusted-server-core/src/integrations/nextjs/script_rewriter.rscrates/trusted-server-core/src/integrations/nextjs/shared.rscrates/trusted-server-core/src/integrations/registry.rscrates/trusted-server-core/src/publisher.rsdocs/guide/integrations/nextjs.mddocs/superpowers/specs/2026-09-07-850-parser-aware-body-hold-nextjs-streaming-design.mddocs/superpowers/plans/2026-09-07-850-parser-aware-body-hold-nextjs-streaming.mdCloses
Closes #850
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(blocked by the existing CommonJS/ESM incompatibility betweenhtml-encoding-snifferand@exodus/bytes; no JS files changed)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test-cloudflare,cargo test-spin,cargo clippy-cloudflare,cargo clippy-cloudflare-wasm,cargo clippy-spin-native, andcargo clippy-spin-wasmChecklist
unwrap()in production code — useexpect("should ...")println!)