Repository navigation
feat(phaser-assets): inspect pack preparation costs before loading - #197
Conversation
The whole pack-preparation budget now runs on one monotonic clock (performance.now) instead of Date.now, so a system clock correction mid-prepare can neither extend nor prematurely exhaust the prepareTimeoutMs budget: - The deadline is computed once per prepare on the monotonic clock and never restarted by a later archive; stagePack's remainder reads the same clock, and the fractional remainder is floored to the decoder's integer deadline contract only after the spent check, so rounding can never mint budget. - No new archive request or decode starts once the monotonic clock says the budget is spent, even when the abort timer's callback is still queued. - A decode that resolves after the budget is spent no longer earns staging handles: the final acceptance gate re-checks the budget and the abort state before handles are acquired. prepareTimeoutMs, requestTimeoutMs and the #191 decode deadline keep their existing responsibilities; user-cancel, dispose and deadline causes stay distinct. Tests drive a manual monotonic clock with gated downloads and a step-driven worker, covering forward and backward wall-clock jumps, one shared budget across dependency archives, download time reducing the decoder budget, zero decode posts after the budget is spent, refusal of success handles on a late decode, and the existing cancel/dispose/files-only regressions.
The pre-request budget gate ran before stagePack invoked the synchronous resolveURL callback, so user code in the resolver could spend the remaining budget while the archive request still started. The gate now sits immediately before the request it guards, after URL resolution, and a regression test advances the fake monotonic clock inside resolveURL: zero archive requests and zero decode posts, deadline failure.
The gated test origin now answers with Connection: close so each phase-gated download uses a fresh connection — a pooled socket reused across gates added runner-dependent transport behavior the test does not measure and intermittently failed the second fetch on CI. The prepare settles through an always-handled wrapper so any transport failure fails the final assertion loudly instead of floating as an unhandled rejection while a gate wait spins.
…details Add delivery.subscribe(listener) as the single observation surface for pack delivery work. Listeners receive prepare and files-delivery read events carrying an operationId (unique per operation within the delivery's lifetime), the operation kind, per-operation sequencing, pack and asset context, the observed phase (planning, downloading, decoding-and-verifying, and exactly one terminal of prepared/completed, failed, cancelled or disposed) and strictly measured progress: network body bytes against the manifest's declared size, verified entry counts and byte totals. Internal hashing/inflate percentages are not observable through the #191 contract and are never fabricated; absent fields mean unknown and per-stage numbers never merge into one synthetic percent. Observation is containment-safe: throwing or rejecting listeners never change results and are never awaited, unsubscribe is idempotent, terminals fire exactly once per operation, and late events from superseded operations are dropped. Files-only prepare stays a light two-event operation (prepared means staging, never images loaded), and staged-entry local reads emit nothing, keeping origin downloads and local reads distinguishable. PhaserPackDeliveryError gains an additive optional details field (stage, operationId, pack/asset identity, httpStatus, decoderStatus/decoderCode, expectedBytes/receivedBytes) captured at the failing execution point; codes, messages and existing constructor calls are unchanged, and no URL or response data ever enters events or details. readCappedDeliveryBody accepts an optional onBodyBytes observation hook. The kit example surfaces the observed phases and numbers beside the loader's own texture progress, keeps stale operation events from overwriting new selections, unsubscribes on scene shutdown (distinct from an explicit cancel), and formats failures from the structured details; the browser acceptance asserts staging observation is distinct from texture readiness.
- The prepare boundary no longer overwrites pack identity the failing
site recorded: a dependency archive failure keeps naming that
dependency in details (regression covered), while correlation fields
still merge in.
- The unsupported worker-environment failure maps to its documented
decoderStatus ('unsupported', not 'error'), and a worker-error decode
failure records decoderStatus 'worker-error'; core digest failures
still leave decoderStatus absent (unknown), per the
absent-means-unknown contract.
- The kit sample renders delivery failures from the stable fields only
(code plus structured details), never message text, and the browser
acceptance matches that display.
- Published docs describe the bounded ZIP decoder instead of the opaque
internal PR number.
Add delivery.inspectPreparation(packId): a read-only cost plan for a pack and its dependency closure, derived from the manifest snapshot and the current staging state. It starts nothing, acquires nothing, reserves nothing, and touches no network, cache, worker or timer. The plan distinguishes the requested pack and revision, the dependency-first closure, per-pack cold artifacts (one object per ZIP archive — entries are not download objects — and one per files-delivery file), the cold object count and manifest body-bytes sum (assuming no caches; not wire bytes), the ZIP packs already staged versus the ones a prepare would stage, the additional staging reservation under the current archive-plus-expanded policy, current and projected staging usage against the configured budget with fitsBudget, and the busy state. Duplicate artifacts follow the manifest's identity rules, a files-only closure reports zero ZIP cost while keeping its full cold file cost, resident textures are never inspected, byte sums are overflow-checked, and the accountingModel identifier pins that the reservation covers staging only — not transport copies, decoder internals, decoded pixels or GPU memory. inspectPreparation and prepare's budget admission share one pure planner (closure + staged set + usage + budget in, plan out), so the published numbers and the enforced admission cannot drift; an inspection is never a reservation. The kit example computes the plan once per selection and shows packs, artifact bytes, projected staging and budget fit next to the observed delivery phases; the browser acceptance asserts the displayed plan agrees with the prepare outcome in both the fits and the one-byte-over budget scenarios.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🧭 Changeset detectedMerging this PR will release the following updates: @mpgd/phaser-assets (npm) — minor version bumpMinor changes
Patch changes
|
![sampo-s-bot[bot]](https://avatars.githubusercontent.com/in/1882503?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe delivery API adds read-only preparation inspection, sequenced progress events, structured error details, and monotonic deadline enforcement. The sample displays these results, and tests cover planning, observation, deadlines, errors, and lifecycle behavior. ChangesPhaser pack delivery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Sample as Asset pack sample
participant Delivery as PhaserPackDelivery
participant Network as Delivery network
participant Decoder as Archive decoder
Sample->>Delivery: inspectPreparation(packId)
Delivery-->>Sample: preparation plan
Sample->>Delivery: prepare(packId)
Delivery->>Network: request archive
Network-->>Delivery: measured body bytes
Delivery->>Decoder: decode with remaining deadline
Decoder-->>Delivery: verified entries
Delivery-->>Sample: progress and terminal events
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit watched the bytes hop by Comment |
Summary
delivery.inspectPreparation(packId)answers, before any download starts, what files a prepare would need and whether it fits the staging budget. Stacked on #195 and #196 — the diff reduces to this feature once those merge to main.API shape — one small read-only method, no CLI planner, no second public planner:
Side-effect free by construction: the planner is a pure helper taking the closure, the staged set, current usage and the budget — no scene, DOM, fetch, worker, cache or timer. A test pins snapshot/requests/worker-posts equality across inspections, and disposed deliveries reject with the existing code.
Shared formula:
prepare's budget admission now runs the same pure planner on live state (replacing its inline copy), so published numbers and enforced admission cannot drift; an inspection is never a reservation, and the exact-boundary/one-byte-over unit test asserts inspect and prepare agree. Already-staged shared dependencies are not re-reserved, and the plan updates correctly after release (staging dropped) and re-inspection.Number semantics: cold bytes assume no HTTP/local cache (not wire bytes); a ZIP is one object; duplicate artifacts follow the manifest's identity rules (never content-hash guessing); files-only closures report zero ZIP cost while keeping their full cold file cost; resident textures are never inspected (no "already loaded so no download" guessing); byte sums are overflow-checked (oversized manifests are config errors, never small-looking costs). The
accountingModelidentifier documents that the reservation covers staging only — excluding transport snapshots/copies, transient Blob conversions, WebCrypto/decoder-internal memory, decoded pixels and GPU resources. No staging-policy changes.Kit example: each selection computes the plan once (never per frame) and shows required packs, artifact bytes, projected staging and budget fit beside the observed delivery phases; the browser acceptance asserts the displayed plan agrees with the prepare outcome in both the fits scenario and the over-budget scenario (
staging=1000), while prepare's internal admission stays authoritative.Verification
pnpm check(incl. validate:public),pnpm build:packages, off-kit packed-tarball consumption (runtime + generated.d.tstypes) pass; localocr review0 findings.Changeset
.sampo/changesets/inspect-pack-preparation.md—@mpgd/phaser-assetsminor.Excluded by design: CLI planner, HTTP/IndexedDB cache probes, auto pack splitting, compression prediction, GPU memory measurement, schedulers, prefetch. Release PR #189 untouched.
Summary by CodeRabbit