Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Extends TS initial-slot ownership from slotRequested to slotRenderEnded in both the runtime and the bootstrap, keeps losing publisher tokens so repeated or late callbacks stay suppressed, and bounds overflow by reusing a denial token at capacity. The change is well scoped, well tested, and matches the spec. The comments below are non-blocking.
2 of the inline comments below carry a one-click GitHub
suggestion. The rest describe the change in prose.
Non-blocking
🤔 thinking
- No bounded recovery if
slotRenderEndednever arrives — see inline atcrates/trusted-server-js/lib/src/core/first_impression.ts:192 - Empty TS render still suppresses the overlapping publisher delivery — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:398
♻️ refactor
- Failing scenario's Playwright trace is never saved — see inline at
crates/trusted-server-integration-tests/browser/initial-render/run.cjs:386
⛏ nitpick
FIRST_IMPRESSION_LEASE_MSdoc comment is now too narrow — see inline atcrates/trusted-server-js/lib/src/core/first_impression.ts:10- Redundant inline
require("node:child_process")— see inline atcrates/trusted-server-integration-tests/browser/initial-render/run.cjs:450
👍 praise
- The browser regression drives real TSJS, Prebid and Universal Creative bundles, checks the actual creative DOM, runs both the runtime-only and bootstrap-first setups, and has a
--baselinemode that shows the unfixed code fails. The unit tests cover every row of the spec's verification table, including callback replay, the capacity overflow, and "a laterslotRequestedcan't reopen the slot".
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (javascript-typescript) ×2: PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- vitest: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- CLAUDE.md symlink guard: PASS
Save the failing scenario's Playwright trace by tracking the per-scenario trace path and context at loop scope and draining them in the existing outer finally, instead of re-indenting the whole scenario body. Use the already-imported execFileSync for the artifact metadata. Document the deliberate scope decisions the review asked about: the TS claim's unbounded expiry when slotRenderEnded never arrives, and empty renders settling identically to filled ones so already-retained denial tokens keep suppressing. Both are recorded in contract item 3 of the design spec and at the code sites. Widen the lease constant's doc comment to cover the auction lease and the pending-render diagnostic delay.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 6f8f1bee. All five earlier comments are addressed:
- The unbounded TS claim and the empty-render suppression are now explicit, documented scope decisions, in code comments and in spec contract item 3. The reasoning holds: element identity and page navigation already retire stale claims, and auctions that start after settlement get no denial token.
- The Playwright trace is now saved on failure via the outer
finally. - The two nit suggestions were applied as proposed.
CI is green, and the touched Vitest suites, the JS build and the browser-page unit tests pass locally.
👍 Nice job keeping the trace fix to about 20 lines instead of a 290-line re-indent, and verifying it with an injected failure.
Optional, non-blocking: the gpt_bootstrap.js mirror of the Number.POSITIVE_INFINITY assignments (lines 232 and 245) could carry the same "deliberately unbounded" comment as the runtime.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 6f8f1bee. The ownership change holds up. Registration now stays open until render. Losing tokens survive consume, release and a repeated callback. At the 16-token limit, overlapping auctions reuse a denial token. Post-render targeting is no longer restored, and the rendered phase can no longer regress. The runtime and bootstrap agree on all of this. The touched Vitest suites (700 tests), the JS build and cargo check-fastly pass locally.
One path the spec names is untested, and it blocks: the pending-render diagnostic after a publisher-to-TS fallback. The rest are small CI, docs and test-harness fixes.
3 of the inline comments below carry a one-click GitHub
suggestion. The other 2 describe the fix in prose because they touch a separate range or other files.
Blocking
🔧 wrench
- Fallback-transition pending diagnostic is untested - see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:186
Non-blocking
♻️ refactor
- A failure in the new browser regression skips the Next.js browser suite - see inline at
.github/workflows/integration-tests.yml:221
📝 note
- Lazy-loaded slots record the pending diagnostic routinely - see inline at
docs/guide/integrations/gpt-diagnostics.md:514
⛏ nitpick
- The bootstrap copy is missing the "deliberately unbounded" comment - see inline at
crates/trusted-server-core/src/integrations/gpt_bootstrap.js:232 pages.test.cjsloads jsdom from another package'snode_modules- see inline atcrates/trusted-server-integration-tests/browser/initial-render/pages.test.cjs:3
CI Status
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS (x2)
- Analyze (actions): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- CLAUDE.md symlink guard: PASS
This branch has not been deployed
4 participants
Summary
A publisher auction starting after the TS-owned slot's
slotRequestedevent but before its initialslotRenderEndedcould issue a second refresh and replace the first creative. Keep initial ownership open through render settlement, retain the original losing disposition when a wrapped callback repeats, and preserve legitimate post-render refreshes.This PR includes the revised spec, implementation plan, runtime/bootstrap changes, unit regressions, and a portable production-bundle browser regression together. The controlled test reproduced two native GPT-fixture calls before the fix and one afterward. Live-site attribution remains unverified: sampled staging loads had no SSAT bids, and supplied server diagnostics reported no-bid/HTTP 400 responses.
Changes
first_impression.ts,gpt_bootstrap.jsbrowser/initial-render/Issue
Closes #1190. This corrects the demonstrated overlap; it does not claim the reported live flicker has been independently reproduced.
Test plan
Fastly/Axum/CLI tests passed after allowing macOS certificate access and local socket binds outside the sandbox.
Deployment note
The five-second expiry no longer releases Trusted Server initial-render ownership.
A missing
slotRenderEndedkeeps competing delivery suppressed until the existingelement/navigation lifetime ends. After five seconds, a one-time diagnostic records
pending state but never unlocks the slot. It is available via debug logging and
pendingRenderDiagnosticon the claim, not the GPT diagnostics panel or JSON export.Publisher fallback leases and diagnostics correlation windows are unchanged.
Scope limits
GPT/GAM selection is simulated in the browser regression; it verifies actual creative DOM through Universal Creative. Arbitrary publisher targeting writes and replay of a consumed pending bid outside the wrapped callback remain outside this correction. A missing initial render event keeps the slot protected until its existing element/navigation lifetime ends. The 16-token bound remains unchanged: overlapping auctions reuse a retained denial token at capacity. Publisher-owned capacity behavior and the existing 256-claim limit remain unchanged. Pending rendering produces a single diagnostic snapshot without releasing ownership.
Checklist