Skip to content

ci(perf): retire the broken transcript data-plane benchmark - #5805

Open
ggbdpq wants to merge 12 commits into
apache:mainfrom
ggbdpq:fix/transcript-benchmark
Open

ggbdpq wants to merge 12 commits into
apache:mainfrom
ggbdpq:fix/transcript-benchmark

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Summary

benchmark:transcript (packages/runtime-host/scripts/transcript-data-plane-benchmark.mjs) crashes on current main: its sqlite reader adapter calls store.readTranscriptPageSnapshot(...) (line 164 of the script), a method #4879 removed when Session transcripts moved to the RuntimeEvent ledger. The storage contract kept only readTranscriptHighWaterSnapshot, so every run dies with TypeError: store.readTranscriptPageSnapshot is not a function before printing any row (reproduced locally on this branch's base; stack below).

Verdict: retire, not repair. Two independent reasons:

  1. There is no drop-in API swap. The script seeded its fixture through store.appendMessages, and rows written that way never reach the RuntimeEvent ledger that the replacement reader (createSessionTranscriptReader) pages from - pointing the same harness at the new reader would page an empty Session and fail its own materialized-count assertion. A repair means rebuilding the fixture through the composition's turn pipeline.
  2. That rebuild already exists. ci(perf): add an end-to-end hydration measurement for long Sessions #5800 adds benchmark:hydration-e2e, which measures the same data plane end to end: a real execution composition, subscription.open with the 16 KiB tail bootstrap, session.transcript.page paging through real handlers, and production ClientSessionSubscription decode - with the storage scan and pager CPU inside each measured round trip. The old script's unique angle (bare sqlite direct reads, arithmetic RTT floor) no longer exists in isolation post-refactor(runtime): derive Session transcripts from RuntimeEvents #4879, since the ledger read path is the read model projection, not a message-table snapshot.

This PR deletes the script and its dead benchmark:transcript package.json entry. History: the script and the API it calls were born together in #2922; #4879 removed the API. Merge-order note: #5800 is still open; until it merges there is a window with no data-plane benchmark, which is acceptable for a script that cannot run at all. Refs #4677 (the hydration budget work that motivated both benchmarks).

Verification

Check Result
Repro of the breakage on base TypeError: store.readTranscriptPageSnapshot is not a function at createSessionTranscriptBootstrap (dist/server/session-transcript-pager.js:33), thrown from the script's sqliteReader adapter
npx biome check packages/runtime-host/package.json clean (no fixes applied)
npm run check:asf-headers 4151 covered / 251 excluded, all carry headers
node scripts/protocol-epoch-check.mjs --staged no protocol changes (epoch 198)
git diff --check --cached clean
Coverage claim: benchmark:hydration-e2e runs on this branch's rebuilt workspace TURNS=2 ROUNDS=1 RTT_MS=1: seed 2s, hostDecodeTail 9.1ms, fullHydration 9.3ms, tail 1.10 KiB / 6 msgs, materialized 6 msgs
Residual references grep -r "benchmark:transcript|transcript-data-plane" over the repo: zero hits after deletion

AI use

Generative tooling - GLM-5.3-Flash (ZCode)

Checklist

The transcript data-plane benchmark called
store.readTranscriptPageSnapshot, which apache#4879 removed when Session
transcripts moved to the RuntimeEvent ledger; the storage contract
kept only readTranscriptHighWaterSnapshot, so the script has crashed
on main ever since.

Retire rather than repair: the script seeded its fixture through
store.appendMessages, and rows written that way never reach the
RuntimeEvent ledger the replacement reader pages from, so pointing the
same harness at the new reader would page an empty Session. A repair
means rebuilding the fixture through the composition's turn pipeline -
which is exactly what apache#5800's benchmark:hydration-e2e already measures
end to end (subscription.open tail bootstrap, session.transcript.page
paging through real handlers, production ClientSessionSubscription
decode). Drop the script and its dead benchmark:transcript entry
rather than duplicate that harness.

Refs apache#4677

Generated-by: GLM-5.3-Flash (ZCode)
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 28, 2026
@ggbdpq

ggbdpq commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

The audit and Build immutable tarball failures on this branch are caused by two new moderate advisories published to the GitHub advisory database on Sep 28 (GHSA-rpw4-54j3-4h4q and GHSA-2vr4-cq9g-pvrc for ip-address, GHSA-3wwx-pv8p-q78v for undici), which the npm audit endpoint picked up between 22:21Z and 23:09Z. Both packages sit in the shipped dependency closure defined by main's lockfile; this PR is a pure deletion (a benchmark script plus its benchmark:transcript script entry) and touches no dependencies - git grep benchmark:transcript on the head tree returns nothing. I reproduced the identical failure (npm audit --omit=dev --workspace maka-agent exiting 1 with moderate: 2, ip-address + undici) on a tree without this PR's diff. Main's nightly tarball build passed at 18:39Z, before the advisories landed; any branch running these checks after ~23Z should be equally red - including the other open branches in this series (#5791, #5800, #5804), whose existing green runs simply predate the advisory sync.

The fix is a lockfile bump (undici to >=6.28.1 / >=7.29.1 / >=8.10.2 depending on the copy; ip-address past 10.5.0 once a patched release lands - the advisory does not mark a first-patched version yet) and belongs on main, not in this PR.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current head against main 2f322055. The only change deletes packages/runtime-host/scripts/transcript-data-plane-benchmark.mjs and its benchmark:transcript package script (packages/runtime-host/package.json:24-29). The deleted harness calls store.readTranscriptPageSnapshot at its former line 164, but SessionStore exposes only the high-water snapshot (packages/storage/src/session-store.ts:691-694); the production transcript reader instead pages the durable ledger (packages/runtime-host/src/server/session-transcript-reader.ts:65-85). I found no remaining repository reference to the deleted benchmark and no substantiated P0-P3 issue in the two-file removal. The package JSON parses, and the fresh-main merge tree and diff check are clean. I did not independently execute the old broken harness or a full local test suite; current-head hosted test passes.

This is not yet a clean merge gate: current-head audit and immutable-tarball checks fail on shipped dependency advisories (ip-address and undici; the tarball's production audit reports two moderate vulnerabilities). This PR does not change the dependency lockfile, so those failures are outside the deletion itself, but they remain red. Also, #5800 is still open and measures a Host-client decode proxy, not the Desktop visible-tail SLO; its small-turn fixture does not replace this deleted harness's large-message/64 MiB stress cases. Please keep that distinction in the performance/acceptance record.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

The two reds on 6ae0d34eb share one root cause and it is not this PR's change (which only deletes a benchmark script and its runtime-host devDependency entry): the CLI release tree resolves undici@7.29.0, and GHSA-3wwx-pv8p-q78v (WebSocket permessage-deflate DoS, 2 × moderate) was published against exactly that version — the audit job flags it and the tarball job's production-dependency audit fails on the same finding. The repository lockfiles still pin undici@6.28.0 (not affected).

undici@7.29.1 — the fixed release — is already on npm, so the tree-identical retrigger at the new head should resolve the patched version and go green. If the resolution still lands on 7.29.0 (semver range permitting), the right fix is an upstream override/pin rather than anything in this PR, and I'll file that separately.

The tarball and audit lanes failed on npm resolving undici@7.29.0 for
the CLI release tree - GHSA-3wwx-pv8p-q78v was published against that
version, and undici 7.29.1 (the fixed release) is out. Tree-identical
retrigger so the installer resolves the patched version.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment: the retrigger did not self-heal, and the reason is now established — the root package-lock.json pins the vulnerable copies (undici@7.29.0 under @ai-sdk/provider-utils, @electron/get, and @slack/socket-mode; undici@6.28.0 at the root), and the release tree installs from the lockfile, so no retrigger changes the resolution.

Fixed on head 879222a02 with per-consumer overrides pinning each undici to the fixed release of its own major (7.30.0 / 6.29.0 / 8.11.2 — all past GHSA-3wwx-pv8p-q78v), and packages/runtime pinned to exactly 7.30.0 because @slack/socket-mode's peer demands ^7 while runtime code needs a working WebSocket/fetch/buildConnector — the gateway and plugin-client-bridge suites pass on 7.30.0 (17/17 and 6/6). One note: unifont's nested undici@8.10.1 was also in the advisory's range and is bumped with the rest, so the next dependency refresh doesn't re-trip the gate.

The affected lanes (audit, Build immutable tarball) should go green on this head.

The release/audit lanes fail because the lockfile pins undici@7.29.0
(three nested copies) and undici@6.28.0, both in the advisory's range;
the release tree installs from the lockfile, so no retrigger helps.
Overrides pin each consumer to the fixed release of its own major
(7.30.0 / 6.29.0 / 8.11.2), and packages/runtime is pinned to exactly
7.30.0 because @slack/socket-mode's peer demands ^7 while runtime code
needs a working WebSocket/fetch/buildConnector (gateway and
plugin-client-bridge tests pass on 7.30.0).

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Second correction, with the missing piece: the overrides update had silently dropped six unrelated resolution entries from the lockfile (gpt-tokenizer, @stylexjs/stylex + its styleq/css-mediaquery/loose-envify peers, and invariant) — the astryx packages still declared those edges, so npm ci refused to install and every npm lane stayed red even though the undici pins were correct.

Head is now a53fceebc: a full npm install re-resolved the six entries (versions match CI's expectations exactly), npm ci passes locally end to end (1047 packages), and the undici pins are unchanged. The audit and tarball lanes should go green on this head.

The undici overrides update left the lockfile with six dangling
dependency edges: gpt-tokenizer, @stylexjs/stylex (plus its styleq /
css-mediaquery / loose-envify peers) and invariant were still declared
by the astryx packages but their node_modules resolution entries had
been dropped, so `npm ci` refused to install at all. A full
`npm install` re-resolved them; `npm ci` now passes locally and the
undici pins are unchanged.

Generated-by: GLM-5.3-Flash (ZCode)
app-builder-lib declares electron-builder-squirrel-windows in its
devDependencies and peerDependencies (exact 26.16.1), and npm's tree
validation refuses the desktop workspace while that peer has no
resolution entry - every npm lane (audit, test, package, tarball) fails
with ELSPROBLEMS. The entry was silently dropped by the same re-
resolution that trimmed the undici lockfile, and being a peer it never
regenerates on its own. Declaring it explicitly in the desktop
devDependencies satisfies the peer durably; npm ci passes with the
workspace tree clean.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Third and (with luck) final piece: the lockfile was still missing one resolution entry — electron-builder-squirrel-windows@26.16.1, which app-builder-lib declares in its devDependencies and peerDependencies (exact version). Being a peer it is invisible to overrides and never regenerates on its own, so the same re-resolution that trimmed the undici entries dropped it too, and npm ls --workspace @maka/desktop --all failed with ELSPROBLEMS — which is what actually killed the audit/test/package lanes (they shell out to npm ls).

Fixed durably by declaring it in the desktop devDependencies (next to electron-builder itself); npm ci passes end to end and the workspace tree validates clean. Head is now bb02913ce. The tarball lane's separate failure (electron release download 500) was transient infrastructure and should clear on this run.

@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Fourth increment on da6af78b5, covering the two new findings on bb02913ce:

  • ip-address@10.4.0 (GHSA-rpw4-54j3-4h4q / GHSA-2vr4-cq9g-pvrc): an override pins 10.7.2, which both declaring ranges (express-rate-limit ^10.2.0, socks ^10.1.1) accept.
  • @stylexjs/stylex@0.19.1 notices error: the MIT copyright override tracked 0.19.0 while the tree resolves 0.19.1 (same upstream, same copyright line) — the key now follows.

The squirrel-windows peer declaration from the previous push is what cleared the ELSPROBLEMS layer these were hiding behind. audit / tarball / test should go green on this head.

…lex notice

The shipped-closure audit now flags ip-address@10.4.0 (GHSA-rpw4-54j3-
4h4q / GHSA-2vr4-cq9g-pvrc, SSRF); an override pins 10.7.2, which both
declaring ranges accept. The third-party-notices generator's MIT
copyright override for @stylexjs/stylex tracked 0.19.0 while the tree
resolves 0.19.1 (same upstream, same copyright line) - the key follows.

Generated-by: GLM-5.3-Flash (ZCode)

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This head also changes production dependencies while retiring the obsolete transcript benchmark. I found two regressions in the added dependency changes. The benchmark removal itself remains sound: it called a SessionStore API that no longer exists. The current head is not ready to merge: hosted test, package, package-linux, and immutable-tarball checks fail, although audit passes. I verified the HTTP-proxy behavior against installed Undici 7.30.0 and 8.10.2, and the stale notices against the current-head hosted logs; I did not run a packaged Desktop or a real external proxy.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread packages/runtime/package.json Outdated
"socks-proxy-agent": "^10.1.0",
"turndown": "^7.2.4",
"undici": "^8.10.2",
"undici": "7.30.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve forward-proxy behavior for HTTP targets when changing Undici majors. buildProxyDispatcher creates new ProxyAgent({ uri, factory, clientFactory }) without proxyTunnel. In Undici 8.10.2, the default forwards HTTP requests to an HTTP proxy; in 7.30.0, proxyTunnel defaults to true, so the same request starts with CONNECT. With this head's installed dependencies, the existing closed successful connections ... (http) network test does not complete within 8 seconds; the same test with Undici 8.10.2 passes in about 80 ms. HTTP forward-only proxies can therefore stop working. Please preserve the previous HTTP behavior (or use an Undici 8 security-fix release) and add a focused proxy regression test. This does not establish failure for proxies that accept CONNECT.

Comment thread package.json
"unifont": {
"undici": "8.11.2"
},
"ip-address": "10.7.2"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Regenerate and commit both Desktop and CLI third-party notices after changing the production dependency graph. Current-head hosted check:third-party-notices reports missing @stylexjs/stylex@0.19.1, ip-address@10.7.2, and undici@6.29.0/7.30.0; the immutable-tarball check:cli-third-party-notices also reports missing ip-address@10.7.2 and undici@7.30.0/8.11.2. Old versions remain listed. This fails the test, package, package-linux, and immutable-tarball gates. Updating the generator's StyleX copyright override alone does not update the checked-in notices.

The ip-address and undici pins moved the shipped closure; the
committed notices files now match it (stylex 0.19.1, ip-address
10.7.2, undici 6.29.0 / 7.30.0 / 8.11.2).

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Fifth increment on b8fbaf00c, three cleanup items surfaced by the last run:

  • Release packager re-keyed: release-cli-package.mjs hardcoded undici@8.10.2 when copying the eval closure and writing the CLI manifest — the tree now resolves 8.11.2, so both references follow.
  • Lockfile registry normalized: the previous re-resolution was run against a local mirror, leaving 36 registry.npmmirror.com resolved URLs where main carries registry.npmjs.org; all rewritten back. Integrity hashes are unchanged (same tarballs).
  • knip exemption: electron-builder-squirrel-windows satisfies app-builder-lib's peer and is never imported by renderer code — exempted in knip.json like the existing fontsource entries, so the knip gate passes with the declaration in place.

audit was already green last round; test / tarball / package / package-linux should follow on this head.

The undici re-key left two references behind: the release packager
hardcoded undici@8.10.2 when copying the eval closure and writing the
CLI manifest (the tree now resolves 8.11.2), and the regenerated
lockfile carried registry.npmmirror.com resolved URLs from a local
mirror config where main's entries all use registry.npmjs.org. knip
also gained an exemption for electron-builder-squirrel-windows: it
satisfies app-builder-lib's peer and is never imported by renderer
code, same as the existing fontsource entries.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Formatting nit from the last push: the knip exemption array exceeded the line width, so biome wants it wrapped. Fixed on head e38204b33 (whitespace-only); only the test job's formatting step needs to re-run.

@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

The remaining test failure was a real undici regression from the aggressive re-key, not flake: on 7.30.0 the eval initialization suite saw an unexpected CONNECT provider.invalid:80 through the ProxyAgent, and scoped-fetch-transport hung on the runtime's 7.30.0. Walked back to behavior-preserving versions on a7969eff4: @ai-sdk/provider-utils → 7.29.1 (the minimal fixed release, patch-level), packages/runtime → its previous 8.10.2 (already past the advisory line). Both previously failing suites pass locally (42/42 and 1/1); every undici copy in the lockfile remains past GHSA-3wwx-pv8p-q78v.

The aggressive undici re-key broke two real consumers: the eval suite's
initialization proxy test saw an unexpected CONNECT through the 7.30.0
ProxyAgent, and the runtime scoped-fetch-transport suite hung on
7.30.0. Walk @ai-sdk/provider-utils back to 7.29.1 (the minimal fixed
release) and packages/runtime back to its previous 8.10.2 (already past
the advisory line); @electron/get keeps 7.30.0 and the 6.x/8.x leaves
keep their resolved versions. Both previously failing suites pass
locally (42/42 and 1/1).

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

One more piece removed on a6df5ef02: the @slack/socket-mode undici override itself was the invalid-marker's author — it demanded 7.30.0 while the runtime tree deduped to 8.10.2, and npm flagged every copy as invalid (ELSPROBLEMS for all npm ls consumers). The lockfile now carries the nested @slack/socket-mode/node_modules/undici@7.30.0 entry directly (fixed release, satisfies the ^7 peer), which is the durable form — the override is redundant against it. npm ls --workspace maka-agent --omit=dev --all validates clean; the lanes should go green on this head.

The `@slack/socket-mode` override demanded undici@7.30.0 while the
runtime tree deduped to 8.10.2, and npm flagged the mismatch as
invalid - which every npm ls consumer (audit, release closure, notices)
turned into ELSPROBLEMS. The lockfile's nested
`@slack/socket-mode/node_modules/undici@7.30.0` entry already satisfies
the ^7 peer with the fixed release, so the override is redundant;
dropping it leaves a fully valid tree.

Generated-by: GLM-5.3-Flash (ZCode)
…losure

The undici version walk-back changed the shipped closure again
(+7.29.1, +8.10.2 after the @slack nested entry re-landed); the
committed notices files now match it, per the lockfile-chain checklist.

Generated-by: GLM-5.3-Flash (ZCode)

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two findings from my previous review are addressed on this head. packages/runtime/package.json restores Undici 8.10.2 for the runtime; after npm ci, the existing HTTP forward-proxy test completes in 74 ms rather than hanging. The regenerated Desktop and CLI third-party notices both pass their local --check commands. The obsolete transcript benchmark remains removed, and I found no new actionable issue in the seven-file increment. The fresh-main merge-tree and diff check are clean; hosted audit, package-linux, and immutable-tarball checks pass.

The hosted test check is still red: an unchanged ACP Goal/Plan child-process test timed out after 30 seconds. I did not reproduce that test locally because this checkout has no built CLI test artifact, so its cause remains unconfirmed. The hosted package check was still running when I submitted this review. This is not merge approval; recheck the current-head required gates before merging. I did not run a real external proxy, a packaged Desktop, or the retired 64 MiB stress scenario.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

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

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants