Repository navigation
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
prk-Jr
changed the title
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This PR adds opt-in, ordered async completion for HTML stream processors at document end. Operators must enable it with publisher.allow_async_html_finish, it shares one deadline, and it keeps the synchronous output if a stage fails, times out or overflows. It also adds timer wiring for Axum, Workers and Spin and a bounded auction suffix. I found no correctness bugs in the runtime path: routing on all four adapters, auction abandonment on init, partial-response and write errors, push_bounded boundary handling, deadline and timer sharing, and the retention overflow replay all check out. The one blocking item is a scope question: this PR turns off shared-template caching where issue #1241 asked for cold-miss support, but its description still says "Closes #1241".
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because it touches multiple files or lines outside the diff, so it can't be auto-applied.
Blocking
❓ question
- Shared-template caching is disabled, but the PR says "Closes #1241": see inline at
crates/trusted-server-core/src/publisher.rs:5108
Non-blocking
🤔 thinking
- The guide's KV example blocks without limit on Fastly: see inline at
docs/guide/integration-guide.md:256 - New AGENTS.md gates 11 to 13 would apply to every PR: see inline at
AGENTS.md:369
♻️ refactor
- Test-only adapter keeps an older API's names alive: see inline at
crates/trusted-server-core/src/integrations/html_finalization.rs:1
📝 note
- The bundled
from_toml_and_envfix duplicates open PR #1326: see inline atcrates/trusted-server-core/src/settings.rs:3083
⛏ nitpick
mod html_finalization;sits between twouselines (suggestion): see inline atcrates/trusted-server-core/src/publisher.rs:23- Test placed above the module's imports: see inline at
crates/trusted-server-adapter-axum/src/platform.rs:612 - Unrelated quote-style churn: see inline at
.github/workflows/test.yml:1
📌 out of scope
- CI now uses two Viceroy versions (0.19 and the pinned 0.17): see inline at
.github/workflows/test.yml:74
👍 praise
- Poisoning replacement writer: see inline at
crates/trusted-server-core/src/streaming_processor/finish.rs:88 - Missing permission fails at startup: see inline at
crates/trusted-server-core/src/integrations/registry.rs:1146 - Boundary-aware marker counting: see inline at
crates/trusted-server-core/src/html_processor/finish.rs:142
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
- vitest: PASS
- cargo fmt: PASS (required)
- HTML EOF runtime (Fastly): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- Analyze (javascript-typescript): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- prepare integration artifacts: PASS
- Analyze (python): PASS
- Analyze (rust): PASS
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed opt-in ordered HTML EOF completion across correctness, architecture, code-smell and style lanes. No verified correctness, architecture or style defects were found. The inline finding is low-priority, non-blocking maintenance feedback, not a demonstrated behavioral defect.
The full review covered a0b00c7..912767c. The subsequent commit 7f1c678 changes only cache documentation; the inline finding was revalidated against that head.
Validation passed: 37 core EOF regressions, 19 finalization tests, 394 publisher tests, 14 runtime-harness helper tests, 18 host-parity tests and 2 documentation fixtures. Counts overlap across filtered core suites. Formatting, whitespace, clock lint, workflow, shell, Markdown and TOML checks also passed. Actual external runtimes and the full CI matrix were not rerun in this review.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This pass covers the six commits since my last review (142a839d9..7f1c67875), plus a fresh read of the EOF runtime path. Every earlier finding is addressed. I found no new correctness, WASM-compatibility or convention problems. On the scope question, I accept the narrower scope with "Closes #1241" kept: shared-template caching for async output is tracked in #1397, and the configuration guide now warns that enabling async finish disables template caching.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it as a commit on the PR branch. The other inline comment is a future-facing note with no change requested in this PR.
Non-blocking
⛏ nitpick
use finish::...splits the external-crate import group (suggestion): see inline atcrates/trusted-server-core/src/streaming_processor.rs:36
🌱 seedling
- Integrations outside core can't unit-test
async_finish: see inline atcrates/trusted-server-core/src/streaming_processor/finish.rs:68
Cross-cutting / body-level findings
- 👍 Every earlier finding was addressed cleanly. The fixtures migrated off the removed test-only adapter declare the same capabilities it did (
requires_async_finish() = true,buffer_output_until_eof() = true), so the regression tests keep their semantics. The guide example now skips its KV lookup when no cooperative timer exists, with the Fastly warning next to the snippet. The EOF checks are scoped inAGENTS.md, the duplicatedfrom_toml_and_envhunk is gone, the workflow quote churn is reverted, and the docs now warn about the template-cache bypass and link #1397. - 🤔 The two range-associated HTML copy paths cap bytes differently (low priority). The sync
stream_publisher_bodycopies range-associated HTML with an uncappedstd::io::copy(crates/trusted-server-core/src/publisher.rs:3046-3051).stream_publisher_body_asynccopies throughBodyChunkSource::with_max_bytes(max_buffered_body_bytes)(publisher.rs:3214-3229), so a 206 HTML response larger than the cap fails partway through. The in-tree adapters don't reach the async copy, because the buffered and Fastly streaming entry points return the raw body before it, so only direct library callers are affected. Consider either dropping the cap on the async pass-through or adding one to the sync path, whichever you intend. This needs a manual change: the two sites are in different hunks, so a singlesuggestioncan't cover both.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- cargo fmt: PASS (required)
- HTML EOF runtime (Fastly): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- vitest: PASS
- Analyze (javascript-typescript): PASS
- prepare integration artifacts: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (cross-adapter parity): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (axum native): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approved. The opt-in ordered EOF completion adds explicit operator permission, bounded fallback handling, and shared deadlines while keeping chunk processing synchronous. No additional comments.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR adds opt-in, ordered async completion for HTML stream processors at document EOF, with operator permission, one shared deadline and bounded fallback. Under probing and mutation testing, the processor lifecycle holds up: one EOF per stage, ordered downstream delivery, deadline ties, candidate poisoning, overflow replay, push_bounded (checked against the old unbounded push on 20,000 random chunkings), and no new work on the no-hook path. Three things block:
- A proven config-blob compatibility break.
- A proven cap bypass on the lazy finalizer for materialized bodies.
- A rebase onto
mainthat needs more than conflict resolution.
10 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. Each one was applied verbatim atf5ec39ca7and checked, individually and as a batch: fmt, all six clippy aliases,test-fastly/axum/cloudflare/spinand parity for the Rust ones; the harness helper tests and clippy for the harness ones; prettier for the docs ones. The remaining comments describe the fix in prose because it spans several hunks or files, or adds tests outside the diff.
Blocking
🔧 wrench
- New
Publisherfields break config rollout and rollback (suggestion): see inline atcrates/trusted-server-core/src/settings.rs:99 - Lazy finalizer skips the raw-size and decode caps for materialized bodies: see inline at
crates/trusted-server-core/src/publisher/html_finalization.rs:132 - Rebasing onto
mainneeds more than conflict resolution: see Cross-cutting below
Non-blocking
♻️ refactor
- Parity test can't catch a wrong
AuctionDispatch::integration_registry: see inline atcrates/trusted-server-integration-tests/tests/parity.rs:1209 - Pre-callback flush barrier has no test (M2 survives), plus two more survivors: see inline at
crates/trusted-server-core/src/publisher/html_finalization.rs:327 - Composite synchronous-EOF guard has no test (M1 survives): see inline at
crates/trusted-server-core/src/html_processor.rs:45 - Document that
requires_async_finishmust not change (suggestion): see inline atcrates/trusted-server-core/src/streaming_processor.rs:58 - The "auction" runtime control never completes a bid round trip: see inline at
crates/trusted-server-integration-tests/tests/html_eof_runtime.rs:713
🌱 seedling
worker::Delayonly wakes its first waker: see inline atcrates/trusted-server-adapter-cloudflare/src/platform.rs:768
🤔 thinking
- The same range bypass reports two abandon reasons: see inline at
crates/trusted-server-core/src/publisher.rs:2657 - The timeout reads as a hard limit on Fastly (suggestion): see inline at
docs/guide/configuration.md:475
📝 note
- Stale draft and scope lines in the spec and plan (suggestion): see inline at
docs/superpowers/specs/2026-10-12-1241-async-html-finalization-design.md:177 - Rustdoc on the async entry points wasn't updated: see inline at
crates/trusted-server-core/src/publisher.rs:3212
⛏ nitpick
max_buffered_body_bytespurpose lost its specifics (suggestion): see inline atdocs/guide/configuration.md:488- Hand-packed
try_stream!body (suggestion): see inline atcrates/trusted-server-core/src/publisher.rs:2687 - Non-atomic
owner.readywrite (suggestion): see inline atcrates/trusted-server-integration-tests/tests/html_eof_runtime/process.rs:434 RUSTUP_TOOLCHAINis dropped (suggestion): see inline atcrates/trusted-server-integration-tests/tests/html_eof_runtime/process.rs:32- Vacuous test (suggestion): see inline at
crates/trusted-server-core/src/streaming_processor/finish.rs:206 1..60000reads as exclusive (suggestion): see inline attrusted-server.example.toml:74
Cross-cutting / body-level findings
- 🔧 Rebasing onto
mainneeds more than conflict resolution.mainis now atf9281917a, and this PR conflicts in.github/workflows/test.ymlandscripts/README.md. The Rust side of the merge is clean: the merged tree's native core suite passes 3045/3045. Two othermainchanges also affect this PR:- #1259 guard. It added "Require the shared Rust setup action" to the required
cargo fmtjob. The newtest-fastly-eofjob callsactions-rust-lang/setup-rust-toolchain@v1directly (line 90 of the merged file), so that required check fails after a plain conflict resolution.- Set the job up with
uses: ./.github/actions/setup-rustand its owncache-key(for examplecargo-fastly-eof). - Move test-axum's
components: clippyinto that action'swith:.
- Set the job up with
- #1123 Viceroy pin.
mainnow pins Viceroy 0.21.0 in.tool-versions, which meets the harness's>= 0.19floor.- Drop the curl, sha256, cache and
$GITHUB_PATHsteps for the separate 0.19 download, and reusetest-rust's cached.tool-versionsinstall. This also closes the earlier dual-Viceroy follow-up. - Rewrite the "isolated Viceroy 0.19" and "ordinary tests retain 0.17" text in
AGENTS.md:373(line 382 after the merge),docs/guide/testing.md:64-71,scripts/README.md:19,34-41, and plan lines 54, 73 and 100. - Keep
validate_viceroy's 0.19 floor; it states the real capability requirement.
- Drop the curl, sha256, cache and
- #1259 guard. It added "Require the shared Rust setup action" to the required
- 📝 Some PR description rows are stale.
- "test-only legacy loader excludes merged build-version metadata": the change was dropped in 6eda72c.
- "compatibility fake callbacks are crate-private and test-only": that module was deleted.
- "Core auction telemetry": no auction file is in the net diff against
main. - "remove … execution histories": see the plan comment.
- "isolated cached Viceroy 0.19 … ordinary 0.17 test pin": changes with the rebase.
- ⛏ Harness and CI.
- Set
command.stdin(Stdio::null())inManagedChild::spawn, so a child that touches the TTY can't stop onSIGTTIN/SIGTTOUin its background process group. - The
cfg(not(unix))arms inprocess.rscan't compile, because libc has no WindowsSIGKILL.#![cfg(unix)]on the test target is simpler and matches the spec. - The test-axum helper and clippy steps omit
--targetwhile the script passes it, so the integration-test graph compiles twice. - No CI job compiles the Cloudflare and Spin
test-utilsFixtureLoggercode.clippy -p trusted-server-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare,test-utils --libandclippy -p trusted-server-adapter-spin --target wasm32-wasip1 --features spin,test-utils --libboth pass today and would keep it from rotting. docs/guide/testing.mdandscripts/README.mdsay SIGTERM, but the harness also handles SIGINT.
- Set
- ⛏ Core.
- The finish marker has two sources of truth:
set_finish_markerstores only its length, while the bytes come fromHtmlFinishContext.marker. Storing the bytes once avoids a caller passing one without the other. - The post-completion expiry checks at
html_processor/finish.rs:218and:252overlap (removing the first survives), and the:252path returns the fallback without logging. Merge them into one final check that keeps the timeoutwarn!. - Removing the
retained.is_some() && !async_capableclause fromvalidatebreaks no test. HtmlWithStreamingProcessorsreturnstruefromrequires_async_finishbut keeps the defaultasync_finish, so it can only be driven throughbegin_finish/finish_next. A line of doc on that impl would help.- Two test names claim more than they check:
terminal_document_state_observes_parser_callbacks_and_isolates_requests(the test builds a fresh state per loop iteration itself) and Axum'shtml_finalization_timer_is_request_scoped(it only asserts the timer is present).
- The finish marker has two sources of truth:
- ⛏ Publisher.
encode_processed_htmltakesOption<&mut AuctionHoldState>, but its only caller passesSome, so theNonebranch is dead.HtmlEofDriver::accepttakesis_last, but its only caller passesfalse.
- ⛏ Docs.
- The guide example omits the factory, although a factory/session capability mismatch fails the request. A short
ExampleFooterFactorywithrequires_async_finish() -> truewould complete it, anddocumentation_snippetswould keep compiling it. allow_async_html_finishcould say that no built-in integration declares async completion yet, so the setting has no effect in a stock build.
- The guide example omits the factory, although a factory/session capability mismatch fails the request. A short
- ⛏ Adapters. Each adapter allocates a new timer
Arcper request, including on no-hook pages. AOnceLocklike the neighbouringGEOand config-store statics avoids it.
CI Status
All checks pass at f5ec39ca7. The PR is now CONFLICTING with main, so these results predate the rebase.
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| fn signal_group(id: u32, signal: i32) { | ||
| #[cfg(unix)] | ||
| if let Ok(id) = libc::pid_t::try_from(id) { | ||
| // Every registered ID belongs to a child started in its own process group. | ||
| unsafe { | ||
| libc::killpg(id, signal); | ||
| } | ||
| } | ||
| #[cfg(not(unix))] | ||
| let _ = (id, signal); | ||
| } |
There was a problem hiding this comment.
⛏ nitpick: The test target is now #![cfg(unix)], so the cfg(not(unix)) arms in this file can no longer compile, and the #[cfg(unix)] guards next to them are redundant. This drops the pair in signal_group:
| fn signal_group(id: u32, signal: i32) { | |
| #[cfg(unix)] | |
| if let Ok(id) = libc::pid_t::try_from(id) { | |
| // Every registered ID belongs to a child started in its own process group. | |
| unsafe { | |
| libc::killpg(id, signal); | |
| } | |
| } | |
| #[cfg(not(unix))] | |
| let _ = (id, signal); | |
| } | |
| fn signal_group(id: u32, signal: i32) { | |
| if let Ok(id) = libc::pid_t::try_from(id) { | |
| // Every registered ID belongs to a child started in its own process group. | |
| unsafe { | |
| libc::killpg(id, signal); | |
| } | |
| } | |
| } |
The same cleanup applies to the #[cfg(not(unix))] block at lines 120-125, the #[cfg(not(unix))] self.child.kill() at lines 189-190, and the #[cfg(unix)] on command.process_group(0) at line 150.
Verified with this applied: rustfmt, the 15 harness helper tests and harness clippy pass.
Summary
Add opt-in, ordered asynchronous HTML processor completion at document EOF through
StreamProcessor::async_finish. Chunk processing remains synchronous. Each stage completes before downstream EOF, and accepted output passes through downstream processing.Factories declare stable async/retention capabilities and require
publisher.allow_async_html_finish = true(default false). Core owns bounded fallback and replacement buffers, one shared deadline, marker validation, and auction/codec ordering. Default EOF settings are omitted from serialized config for compatibility with older binaries. All async input representations share cumulative raw and decoded byte limits.Non-retaining streaming processors can release ready HTML before optional work. Retention delays initial delivery; Axum, Workers and Spin currently buffer publisher responses. Limits apply per holder, not total request memory. Axum/Workers/Spin timers interrupt yielding work. Fastly checks the budget before and after callbacks and cannot preempt blocking host calls; integrations must bound those calls themselves.
Async capability bypasses processed-template caching before lookup; processed output uses
Cache-Control: no-store, private, including fallback. Raw-origin readthrough stays independent. Range-associated HTML preserves encoded bytes and origin metadata without EOF work. Shared-template caching for async output is tracked in #1397.The feature-gated native runtime harness checks disabled/success/failure/timeout behavior and Fastly early-byte delivery. Its auction control intentionally uses an unreachable bidder and verifies empty-bid script assembly after finalized output; it does not establish successful or delayed bid collection. CI uses the repository-pinned Viceroy shared with ordinary tests and the shared Rust setup action. WASM Clippy also covers Workers/Spin fixture loggers.
Review remediation
RUSTUP_TOOLCHAIN, publish readiness atomically, and gate Unix process tooling.mainin a separate commit and reconcile its toolchain, shared setup action, Viceroy and documentation changes.API migration
Existing processors inherit default methods. Opted-in factories and sessions must declare matching lifetime-stable capabilities. Direct
AuctionDispatchconstruction must pass the sameintegration_registryused for response processing.Publisherstruct literals need both new fields or..Publisher::default().Validation
Fresh results on
423682810:Local sandbox certificate/socket restrictions and transient shared-target doctest metadata were resolved by permission/fresh-run retries; no source changes were needed for those runner issues. Runtime auction controls verify intentional empty-bid assembly, as described above.
Closes #1241.