starred/vinext

Fork 0

mirror of https://github.com/cloudflare/vinext.git synced 2026-05-09 08:25:34 +02:00

[GH-ISSUE #479] Phase 3/3: Cleanup dual-path ALS and evaluate per-call scopes #107

New issue

Closed

opened 2026-05-06 12:37:15 +02:00 by BreizhHardware · 1 comment

BreizhHardware commented

2026-05-06 12:37:15 +02:00

Owner

Originally created by @Divkix on GitHub (Mar 11, 2026).
Original GitHub issue: https://github.com/cloudflare/vinext/issues/479

Problem

After Phases 1 and 2, all per-request state will be consolidated into UnifiedRequestContext. However, there remain ALS scopes that are intentionally per-call, not per-request:

cacheContextStorage (cache-runtime.ts:61) — Tracks cache context for individual cache() calls
_unstableCacheAls (cache.ts:598) — Tracks whether execution is inside unstable_cache()

These exist outside the unified scope because:

They nest inside per-request scope (a single request may make multiple cache() calls)
Each cache() call has its own context (tags, revalidate, cache life)
They need to be independently cleanable without affecting request-scoped state

Additionally, Phase 1 and 2 leave behind dual-path code — standalone ALS instances that exist only for backward compatibility. Once the unified scope is proven stable, these can be removed.

Goal

Evaluate whether per-call ALS scopes can/should be consolidated
Clean up dual-path fallback code once unified scope is ubiquitous
Document the final ALS architecture clearly

Phase 3A: Evaluate Per-Call Scope Consolidation

Current State

// cache-runtime.ts — per-cache-call ALS
export const cacheContextStorage = new AsyncLocalStorage<CacheContext>();

// cache.ts — per-unstable_cache-call ALS  
const _unstableCacheAls = new AsyncLocalStorage<boolean>();

Questions to Answer

Should these be consolidated into unified context?
- Option A: Keep separate — they're semantically different (call vs request)
- Option B: Nest in unified — add currentCacheContext field to UnifiedRequestContext
- Option C: Hybrid — unified for request, separate for nested calls
Performance impact?
- Measure ALS overhead for deeply nested cache() calls
- Determine if consolidation reduces or increases overhead
Correctness implications?
- Can we safely reset cache context between calls without affecting request state?
- Do nested cache() calls need isolation from each other?

Spike Tasks

Benchmark: nested cache() calls with current separate ALS
Benchmark: nested cache() calls with unified approach
Verify no state leakage between concurrent cache calls
Document decision and rationale

Phase 3B: Cleanup Dual-Path Code

What Gets Removed

After Phases 1 and 2 are stable (suggest waiting 2-4 weeks), remove standalone ALS fallbacks:

headers.ts
- Remove _als standalone instance
- Remove _fallbackState object
- Keep isInsideUnifiedScope() check, but fail/throw if false in production
navigation-state.ts
- Remove _als standalone instance
- Remove _fallbackState object
cache.ts
- Remove _als standalone instance (for cache state)
- Remove _fallbackState object
cache-runtime.ts
- Keep cacheContextStorage (per-call, not per-request)
- Remove _privateCache standalone ALS (unified has this now)
fetch-cache.ts
- Remove _als standalone instance
- Remove _fallbackState object
request-context.ts
- Remove _als standalone instance (for execution context)
router-state.ts (after Phase 2)
- Remove _als standalone instance
- Remove _fallbackState object
head-state.ts (after Phase 2)
- Remove _als standalone instance
- Remove _fallbackState object

What Stays (and why)

ALS	Purpose	Why Kept
`cacheContextStorage`	Per-call cache context	Semantically different scope level
`_unstableCacheAls`	Per-call unstable_cache tracking	Same as above
`_als` in `unified-request-context.ts`	The unified scope	Core infrastructure

Phase 3C: Documentation

Architecture Document

Create docs/als-architecture.md explaining:

Unified scope — One ALS for all per-request state
Per-call scopes — Separate ALS for nested cache calls
SSR/client separation — Why separate Vite environments need care
Migration guide — For contributors adding new state

Code Comments

Update each shim module with:

/**
 * Unified scope is the primary path (Phase 1/2).
 * Standalone ALS is deprecated and will be removed in Phase 3.
 * @deprecated Standalone fallback to be removed after Phase 2 stabilization.
 */

Risks and Mitigations

Risk	Mitigation
Breaking external code	This is internal API; external code shouldn't use ALS directly
Test breakage	Ensure all tests use unified scope before removing fallbacks
Hard to debug	Add clear error messages when unified scope is expected but missing
Performance regression	Benchmark before/after; keep per-call scopes if needed

Decision Gates

Gate 1: Phase 3A approval

Benchmark results reviewed
Decision documented (consolidate vs keep separate)
Implementation plan approved

Gate 2: Phase 3B timing

Phase 1 in production for 2+ weeks with no issues
Phase 2 in production for 2+ weeks with no issues
All tests migrated to unified scope
Deprecation warnings added in Phase 2

Phase 1: #451 (App Router consolidation)
Phase 2: #XXX (Pages Router consolidation) — will create this
Unified context: shims/unified-request-context.ts

Estimated Effort

Phase 3A: Small — research and benchmarking
Phase 3B: Medium — code deletion and test updates
Phase 3C: Small — documentation writing

Total: Medium — mostly waiting for stabilization between phases.

Originally created by @Divkix on GitHub (Mar 11, 2026). Original GitHub issue: https://github.com/cloudflare/vinext/issues/479 ## Problem After Phases 1 and 2, all per-request state will be consolidated into `UnifiedRequestContext`. However, there remain ALS scopes that are intentionally **per-call**, not per-request: 1. **`cacheContextStorage`** (`cache-runtime.ts:61`) — Tracks cache context for individual `cache()` calls 2. **`_unstableCacheAls`** (`cache.ts:598`) — Tracks whether execution is inside `unstable_cache()` These exist outside the unified scope because: - They nest inside per-request scope (a single request may make multiple `cache()` calls) - Each `cache()` call has its own context (tags, revalidate, cache life) - They need to be independently cleanable without affecting request-scoped state Additionally, Phase 1 and 2 leave behind dual-path code — standalone ALS instances that exist only for backward compatibility. Once the unified scope is proven stable, these can be removed. ## Goal 1. **Evaluate** whether per-call ALS scopes can/should be consolidated 2. **Clean up** dual-path fallback code once unified scope is ubiquitous 3. **Document** the final ALS architecture clearly ## Phase 3A: Evaluate Per-Call Scope Consolidation ### Current State ```typescript // cache-runtime.ts — per-cache-call ALS export const cacheContextStorage = new AsyncLocalStorage<CacheContext>(); // cache.ts — per-unstable_cache-call ALS const _unstableCacheAls = new AsyncLocalStorage<boolean>(); ``` ### Questions to Answer 1. **Should these be consolidated into unified context?** - Option A: Keep separate — they're semantically different (call vs request) - Option B: Nest in unified — add `currentCacheContext` field to `UnifiedRequestContext` - Option C: Hybrid — unified for request, separate for nested calls 2. **Performance impact?** - Measure ALS overhead for deeply nested `cache()` calls - Determine if consolidation reduces or increases overhead 3. **Correctness implications?** - Can we safely reset cache context between calls without affecting request state? - Do nested `cache()` calls need isolation from each other? ### Spike Tasks - [ ] Benchmark: nested `cache()` calls with current separate ALS - [ ] Benchmark: nested `cache()` calls with unified approach - [ ] Verify no state leakage between concurrent cache calls - [ ] Document decision and rationale ## Phase 3B: Cleanup Dual-Path Code ### What Gets Removed After Phases 1 and 2 are stable (suggest waiting 2-4 weeks), remove standalone ALS fallbacks: 1. **headers.ts** - Remove `_als` standalone instance - Remove `_fallbackState` object - Keep `isInsideUnifiedScope()` check, but fail/throw if false in production 2. **navigation-state.ts** - Remove `_als` standalone instance - Remove `_fallbackState` object 3. **cache.ts** - Remove `_als` standalone instance (for cache state) - Remove `_fallbackState` object 4. **cache-runtime.ts** - Keep `cacheContextStorage` (per-call, not per-request) - Remove `_privateCache` standalone ALS (unified has this now) 5. **fetch-cache.ts** - Remove `_als` standalone instance - Remove `_fallbackState` object 6. **request-context.ts** - Remove `_als` standalone instance (for execution context) 7. **router-state.ts** (after Phase 2) - Remove `_als` standalone instance - Remove `_fallbackState` object 8. **head-state.ts** (after Phase 2) - Remove `_als` standalone instance - Remove `_fallbackState` object ### What Stays (and why) | ALS | Purpose | Why Kept | |-----|---------|----------| | `cacheContextStorage` | Per-call cache context | Semantically different scope level | | `_unstableCacheAls` | Per-call unstable_cache tracking | Same as above | | `_als` in `unified-request-context.ts` | The unified scope | Core infrastructure | ## Phase 3C: Documentation ### Architecture Document Create `docs/als-architecture.md` explaining: 1. **Unified scope** — One ALS for all per-request state 2. **Per-call scopes** — Separate ALS for nested cache calls 3. **SSR/client separation** — Why separate Vite environments need care 4. **Migration guide** — For contributors adding new state ### Code Comments Update each shim module with: ```typescript /** * Unified scope is the primary path (Phase 1/2). * Standalone ALS is deprecated and will be removed in Phase 3. * @deprecated Standalone fallback to be removed after Phase 2 stabilization. */ ``` ## Risks and Mitigations | Risk | Mitigation | |------|------------| | Breaking external code | This is internal API; external code shouldn't use ALS directly | | Test breakage | Ensure all tests use unified scope before removing fallbacks | | Hard to debug | Add clear error messages when unified scope is expected but missing | | Performance regression | Benchmark before/after; keep per-call scopes if needed | ## Decision Gates **Gate 1: Phase 3A approval** - [ ] Benchmark results reviewed - [ ] Decision documented (consolidate vs keep separate) - [ ] Implementation plan approved **Gate 2: Phase 3B timing** - [ ] Phase 1 in production for 2+ weeks with no issues - [ ] Phase 2 in production for 2+ weeks with no issues - [ ] All tests migrated to unified scope - [ ] Deprecation warnings added in Phase 2 ## Related - Phase 1: #451 (App Router consolidation) - Phase 2: #XXX (Pages Router consolidation) — will create this - Unified context: `shims/unified-request-context.ts` ## Estimated Effort - Phase 3A: Small — research and benchmarking - Phase 3B: Medium — code deletion and test updates - Phase 3C: Small — documentation writing **Total: Medium** — mostly waiting for stabilization between phases.

BreizhHardware closed this issue

2026-05-06 12:37:15 +02:00

BreizhHardware commented

2026-05-06 12:37:17 +02:00

Author

Owner

@Divkix commented on GitHub (Mar 11, 2026):

Closing this as a standalone phase. The ALS rollout is now being tracked as a 2-issue plan: #451 / PR #450 for the rollout itself, and #478 for the remaining parity tests, cleanup, per-call ALS evaluation, and documentation. The remaining useful parts of this issue are folded into #478.

@Divkix commented on GitHub (Mar 11, 2026): Closing this as a standalone phase. The ALS rollout is now being tracked as a 2-issue plan: `#451` / PR `#450` for the rollout itself, and `#478` for the remaining parity tests, cleanup, per-call ALS evaluation, and documentation. The remaining useful parts of this issue are folded into `#478`.

BreizhHardware referenced this issue

2026-05-06 12:39:09 +02:00

[PR #107] [MERGED] fix: handle malformed percent-encoded URLs gracefully #316