Skip to content

WebGPU modelLoadFromUrl is a 408-line function whose retry ladder restarts itself by mutating a loop counter #361

Description

@leehack

Found during a code-health audit of main, confirmed by a follow-up verification pass. No fix applied.

Issue #354 lists webgpu_backend.dart in its file-size table, but its problem statement and all three suggested steps are about LlamaCppService and native-library discovery. This function is never described there, so filing separately.

What

WebGpuLlamaBackend.modelLoadFromUrllib/src/backends/webgpu/webgpu_backend.dart:971-1378, 408 lines.

Before the loop it validates params, resolves preferMemory64, resolves thread counts, applies a Safari GPU-layer clamp via _resolveSafeRequestedGpuLayers, and marshals the progressCallback across the JS boundary (:976, :981, :986-988, :991-1003, :1019-1038). Then it runs an attempt ladder from _buildLoadAttempts (:1041, loop at :1056) whose catch block escalates.

The part that makes it hard to change

Six retry branches, each ending in continue;:1240, :1261, :1277, :1302, :1321, :1350 — behind five named predicates (shouldRetryWithSmallerRemoteFetchChunks, shouldRetryWithoutRemoteFetchBackend, shouldRetryWithWasm32, shouldRetryWithWasm64, canRetry) plus one inline fsWriteFailed && coreVariant == 'wasm64'.

Five of the six restart the entire ladder by assigning index = -1:1232, :1251, :1271, :1288, :1314 — mutating a for loop's induction variable from inside a catch. The escalation state is carried in seven mutable locals declared at :1048-1054 (retriedWithWasm32, retriedWithWasm64, retriedWithoutRemoteFetchBackend, remoteFetchChunkRetryCount, retriedAfterFsWriteFailureWithRemote, remoteFetchBackendKnownUnstable, wasm64InteropKnownBroken) plus two instance fields (_preferMemory64Override, _forceRemoteFetchBackendOverride) mutated at :981, :1030 and inside four branches.

To be clear about what this is not: it terminates. Every restart is latched by its own boolean and chunk-halving is capped at 10. This is a testability and maintainability problem, not a hang risk.

Half the ladder is untested

Branch Coverage
wasm64 memory-pressure retry Covered — webgpu_engine_multimodal_browser_integration_test.dart:317 asserts modelLoadCallCount == 2
wasm64 FS-write, both arms Covered — same file, :334, :365
shouldRetryWithSmallerRemoteFetchChunks No test
shouldRetryWithoutRemoteFetchBackend No test
shouldRetryWithWasm32 (BigInt interop) No test
canRetry ladder-advance No test

Repo-wide greps for bigint, model_fetch_backend_abort, model_fetch_backend_skipped_small and thread_constructor_failed across test/ return zero hits. The covered branches do run in CI (.github/workflows/ci.yml:218, dart test -p chrome --exclude-tags local-only; the file is @TestOn('browser')).

dart analyze reports no issues here, so nothing guards against it growing further.

A duplication to fix while you are in there

The index-to-thread-count switch at :1067-1074 is copy-pasted verbatim into a log-string interpolation at :1343-1348, purely to print the number it already computed.

Suggested direction

_buildLoadAttempts (:632-671) already models the attempt sequence as data, so the usual "make it a table" advice is half-done. The part that is not data-driven is the failure-classifier and escalation layer above it.

  1. Extract the pre-loop setup (validation, thread resolution, Safari clamp, callback marshalling) into its own function so the ladder starts at the top of the body.
  2. Replace index = -1 with an explicit restart signal returned by a classifier — e.g. a function mapping (error, state) -> RetryDecision{advance | restartWith(overrides) | giveUp} — so the escalation policy becomes unit-testable without a browser.
  3. Add coverage for the four untested branches once they are reachable from a test seam.
  4. Collapse the duplicated thread-count switch.

Note on the audit's numbers

The original audit said "seven-branch". Six is the defensible count (six continue; statements); seven is reachable only by also counting the terminal _normalizeBridgeRuntimeError/rethrow path or the non-retrying state mutation at :1185. It also said 410 lines; measured span is 408.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpriority:P3Watch or strategic work blocked by upstream/runtime/design dependencies

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions