Fix: select the client-label context store at runtime so the web build drops node:async_hooks - #660
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: select the client-label context store at runtime so the web build drops node:async_hooks#660AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 4, 2026 14:54
client_labels.ts statically imported node:async_hooks, and the module is reachable from the browser entry point index_web.ts. The web build is transpile-only, so the specifier survived verbatim into dist/web and no browser bundler could resolve it. Default the store to the existing synchronous single-slot shim and add an internal setter seam. index.ts, the Node entry point, installs a real AsyncLocalStorage through a new client_labels_node.ts, which is now the only module in core/src that names node:async_hooks and is unreachable from index_web.ts.
Adds shim coverage (it is now the live browser fallback), a Node-store test for AsyncLocalStorage semantics, and, in client_labels_test.ts, a concurrency invariant guard, a nesting/restore case, a fake-store test for the new setter seam, and an esbuild browser-bundle regression test that fails while client_labels.ts names node:async_hooks.
…edupe the invariant The build.js browser alias for node:async_hooks never fires: packages: 'external' (build.js:49) externalizes the bare specifier before the alias is consulted. Verified by injecting a browser-reachable node:async_hooks import and building with --bundle -- the specifier survives verbatim into dist/web/index.js with the alias present. It therefore gave a false impression that browser bundles were protected from node:async_hooks when they were not, and after the runtime store seam nothing browser-reachable imports it at all. Removing it changes no build output: dist/web is byte-identical either way. Also collapses the browser-graph invariant to a single statement on client_labels_node.ts, the module that owns it.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Related: Fix: remove orphaned fragment from BasePlugin.onToolErrorCallback JSDoc #611
Related: Fix: keep temp: state readable for the whole invocation in DatabaseSessionService (stacked on #132) #607
(Deliberately not
Closes: #611— that issue also covers the winston half, which is a separate change, and #614 also claims to close it.)Problem:
core/src/utils/client_labels.tsstatically imported a Node builtin:That module is reachable from the browser entry point (
core/src/index_web.ts→./common.js→core/src/common.ts:282). The published web build is transpile-only —core/package.jsonruns"build": "tsc --emitDeclarationOnly && node ./build.js"with no--bundle, socore/build.jstakes theentryPoints: ['./src/**/*.ts']+outdirbranch and passes every module specifier through verbatim. The specifier therefore survived intocore/dist/web/utils/client_labels.js:8, and no browser bundler can resolve it, even thoughcore/package.jsonadvertises"browser": "./dist/web/index_web.js".A shim already existed at
core/src/utils/async_hooks_shim.ts, but it was wired only through esbuild'sbuildOptions.alias, gated onplatform === 'browser' && bundle(core/build.js:53). That gate is load-bearing: esbuild fails outright withCannot use "alias" without "bundle", so the alias never applied to the shipped (non-bundled) web build.Solution: select the store at runtime instead of at build time.
core/src/utils/client_labels.tsno longer names anynode:module. It declares a minimal internalClientLabelStoreinterface (run+getStore), defaults to the existing synchronous shim, and exposes an internalsetClientLabelStore()seam. The shim is imported under the aliasSingleSlotClientLabelStorebecause its exported class is literally namedAsyncLocalStoragebut is not one — importing it under its own name at that site invites exactly the confusion this bug is about.core/src/utils/client_labels_node.tsinstalls Node's realAsyncLocalStorage. It is now the only module incore/srcthat namesnode:async_hooks, and nothing reachable fromindex_web.tsimports it.core/src/index.ts(the Node entry point) callsinstallNodeClientLabelStore()at module-evaluation time, before any user code.core/src/utils/async_hooks_shim.tsgains a class doc comment only. It was dead code before this change and is now the live browser fallback, so it says plainly that it is a synchronous single-slot stand-in, that the value does not survive anawait, and that it must never become the Node implementation. Behaviour, export name and file name are unchanged.core/build.jsloses the five-lineif (platform === 'browser' && bundle)alias block, which contained thenode:async_hooksentry and nothing else. That alias never fired.packages: 'external'(core/build.js:49) externalizes the barenode:async_hooksspecifier before the alias is consulted, so even a genuinely browser-reachable import was never substituted. Measured by injecting exactly such an import into a module reachable fromindex_web.tsand runningnpm run build:bundle: with the alias present, the specifier survives verbatim asimport {AsyncLocalStorage} from 'node:async_hooks';atdist/web/index.js:23. The block therefore gave the false impression that browser bundles were protected fromnode:async_hookswhen they were not — and after the runtime seam nothing browser-reachable imports it in any case. Removing it changes no build output:dist/webis identical with and without it, and bothnpm run buildandnpm run build:bundlestill succeed.Node's
AsyncLocalStorage<string>is structurally assignable toClientLabelStore, so there is no cast, noany, and no suppression anywhere in this diff.No public API change.
core/src/common.ts:282keeps its existing export list (getClientLabels,runWithClientLabel);ClientLabelStore,setClientLabelStoreandinstallNodeClientLabelStorestay internal. No signature changes, no new dependency —package.jsonandpackage-lock.jsonare untouched.Corrected acceptance criteria (please read — the obvious one is not achievable)
The natural criterion,
cd core && npm run build && grep -rn 'node:async_hooks' dist/webreturning 0 hits, cannot be met by any source-only change, and this was verified rather than argued:core/build.jsemits every file incore/srcintocore/dist/web, reachable fromindex_web.jsor not. A whole-tree grep therefore hits unreachable files too —dist/webalready ships unreachablenode:fs,node:path,node:net,node:dns/promises,node:cryptoandnode:osspecifiers today for the same reason.AsyncLocalStoragewithout naming the module. Checked with esbuild:import typeis fully erased, but'node:' + 'async_hooks'is constant-folded straight back to"node:async_hooks", and a dynamicimport('node:async_hooks')keeps the literal too. Splitting the specifier to dodge a grep would be obfuscation, not a fix, and was not attempted.Measured against these criteria instead (base
693a1d79,corev1.5.0):node:async_hooksresolve errors bundlingdist/web/index_web.jsfor the browsergrep -c 'node:async_hooks' dist/web/utils/client_labels.jsdist/webstill containing the literalutils/client_labels.jsutils/client_labels_node.js(unreachable, Node-only)npx esbuild src/utils/client_labels.ts --bundle --platform=browserThe last residual hit closes only when the web build stops emitting unreachable modules — a build-system change (#614, or a follow-up), not this PR.
One honest correction to the expected numbers. The total error count bundling
dist/web/index_web.jsstays at 346, it does not drop to 345. Thenode:async_hookserror disappears (1 → 0), butasync_hooks_shim.jsbecomes newly reachable from the browser graph and carries the pre-existingcreateRequirepreamble thatcore/build.js:73-76prepends to every emitted file, soCould not resolve "module"goes 147 → 148. That banner defect is pre-existing, unrelated, and separately tracked; this PR does not make it worse in kind, only in count by one, and fixing it means touchingcore/build.js, which is out of scope here.Measured taint reduction (source-level,
esbuild --bundle --platform=browser, error counts)core/src/models/base_llm.tscore/src/utils/client_labels.tscore/src/agents/llm_agent.tscore/src/runner/runner.tscore/src/models/registry.tscore/src/tools/agent_tool.tsThe residual 20 are winston (
util,os,fs,path, …) reached throughcore/src/utils/logger.ts. Dropping winston is a separate in-flight change; only once both land dollm_agent.tsandrunner.tsreach zero. This PR alone does not make the web build bundle — it removes one of the two causes, and it is what unblocks the web-specific barrel work in #612 (of the 22 Node-tainted re-exports incore/src/common.ts, 15 were tainted solely by this single import, includingLlmAgent,Runner,BaseLlm,GoogleLlm, the model registry,AgentToolandSequentialAgent).Relationship to #614
Open PR #614 ("Fix: make the web build bundle and run in a browser") also references #611 and solves the same symptom a different way: it makes the web target always bundle, which is what makes esbuild's
aliaslegal so the existing shim finally applies. Confirmed stillOPENat the time of writing.core/build.js, and are otherwise disjoint. Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in GoogleSearchTool #614 touchescore/build.js,core/src/common.ts,core/src/index.ts(export block only),core/src/utils/winston_shim.tsandtests/integration/build_setup/web_build_test.ts. This PR touchescore/src/utils/client_labels.ts,core/src/utils/client_labels_node.ts,core/src/utils/async_hooks_shim.ts,core/src/index.ts(one import + one call, no exports),core/build.js(deleting the five-line dead alias block) andcore/test/utils/*.client_labels.ts's import. The measurement above shows the alias does not actually work underpackages: 'external', so that premise does not hold as stated; and once this PR lands,client_labels.tshas no such import to substitute. If Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in GoogleSearchTool #614 lands after this one itscore/build.jshunk needs a trivial rebase around the deleted block, and its alias-based mechanism is redundant rather than conflicting.client_labels.tsis browser-safe under any build strategy.tests/integration/build_setup/— Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in GoogleSearchTool #614 ownsweb_build_test.tsthere. The regression coverage lives incore/test/utils/instead.Deliberate deviation from the agreed plan. The plan for this change said to leave
core/build.jsalone and queue the alias removal as a follow-up, to avoid colliding with #614. A complexity review flagged the leftover block; on measuring it I agreed and removed it. It is not merely dead — it never functioned, and a non-functional guard is worse than no guard because it reads as protection. The deletion is five lines, contains only thenode:async_hooksentry, and provably changes no build output.Collision check. Searched all 557 open PRs on the fork (
gh pr list --limit 1000, plus--search async_hooks/client_labels/611). No open PR touchescore/src/utils/client_labels.tsorcore/src/utils/async_hooks_shim.ts. The nearest neighbours were checked by file list and are disjoint: the winston-removal PR (logger.ts,env_aware_utils.ts,file_utils.ts, artifacts, code executors), the package-exports PR (core/package.json,core/test/package_exports_test.ts), and thegoogle_llm.tslabels-cast PR. Branched frommain, not stacked.Known, accepted limitation
Because the install is wired through
core/src/index.ts, any in-repo code path that reachesclient_labels.tswithoutindex.tshaving been evaluated gets the synchronous fallback rather thanAsyncLocalStorage. This is measured, not assumed. In practice every entry into the package goes throughindex.ts:core/package.jsonexportsexposes only"."(no subpath exports, so an external consumer cannot reachclient_labels.jsany other way), the vitest@google/adkalias resolves tocore/src/index.ts, and thedevandintegrationsworkspaces were checked for deep imports of core internals — there are none. Behaviour for Node consumers is unchanged; browser behaviour improves from "cannot load at all" to "loads, with single-slot label context". Reaching realAsyncLocalStoragefromclient_labels.tsitself is impossible without namingnode:async_hooksthere, which is the whole defect.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
11 new tests across three files. Test placement follows the source layout, with each behaviour pinned once:
core/test/utils/async_hooks_shim_test.ts(new, 5 tests). The shim had zero tests and is now the live browser fallback: initial empty store, value visible insiderunplus return-value pass-through, nesting/restore, restore-on-throw (thefinally), and an explicit assertion of the documented limitation that the value does not survive anawait, so nobody later mistakes it for anAsyncLocalStorageequivalent.core/test/utils/client_labels_node_test.ts(new, 2 tests). CallsinstallNodeClientLabelStore()explicitly, then asserts the label survives anawaitand that concurrent invocations stay isolated. This is the only coverage ofcore/src/utils/client_labels_node.ts.core/test/utils/client_labels_test.ts(4 tests added; all 17 existing tests untouched). A concurrency invariant guard and a nesting/restore case through the public API, a fake-ClientLabelStoretest for the new setter seam, and an esbuild browser-bundle regression test.The concurrency case deliberately appears in two files, and they are not duplicates: the one in
client_labels_node_test.tsinstalls the store itself and pins the store, while the one inclient_labels_test.tsrelies only on importing@google/adkand therefore pins the wiring inindex.ts. Mutation M2 below demonstrates the difference — removing theindex.tscall fails one and not the other.Commands run (targeted, on the exact commit pushed):
npx vitest run --project unit:dev --project unit:integrationsreports 2 failures —integrations/test/version_test.ts(expected '1.5.0' to be '1.3.0', a stale hardcoded version assertion) anddev/test/cli/cli_create_test.tsgcloud-defaults. Both were reproduced on the unmodified base commit withcore/srcreverted, so they are pre-existing and unrelated; they are not touched here.Coverage.
async_hooks_shim.tsandclient_labels_node.tsare at 100% of statements, branches, functions and lines.client_labels.tsis at 98.3% statements / 93.33% branches; the single uncovered line is the pre-existingparseUserAgent(window.navigator.userAgent)browser-only branch at line 71, which this change does not touch and which cannot execute under thenodetest environment.Proof the tests can fail. Every new test was run against mutated source and observed to fail. Each mutation was reverted immediately (
git checkout --) and the tree verified clean.node:async_hooksimport as the default store inclient_labels.ts(i.e. the bug)Build failed with 1 error: core/src/utils/client_labels.ts:8:32: ERROR: Could not resolve "node:async_hooks"installNodeClientLabelStore();fromindex.tsclient_labels_test.ts(+ the pre-existing async-hops test);client_labels_node_test.tsstill passedexpected [ undefined, undefined ] to deeply equal [ 'task-a', 'task-b' ]client_labels_node.tsimports from./async_hooks_shim.js) — the thing the invariant forbidsclient_labels_node_test.tstestsexpected undefined to be 'task-a';expected [ undefined, undefined ] to deeply equal [ 'task-a', 'task-b' ]try/finallyrestoreexpected [ 'outer', 'inner', 'inner', 'inner' ] to deeply equal [ Array(4) ]runnever stores (this.store = previous)expected undefined to be 'a'getStorenever reports emptyexpected 'seeded' to be undefinedsetClientLabelStorea no-opexpected [] to deeply equal [ 'routed-label' ]runWithClientLabelbypasses the store (return callback())expected [ undefined, undefined, …(2) ] to deeply equal [ Array(4) ]Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.