fix(netTransform): Beam to IDE — live beam node, JSON artifact cache, throttle cooldown, reaped-beam drop-off - #2883
Open
Rajanna-Karthik wants to merge 7 commits into
Open
Rajanna-Karthik wants to merge 7 commits into
Rajanna-Karthik wants to merge 7 commits into
Conversation
…ing them Beam discovery re-scans the same candidate artifacts on every sweep, and each candidate was downloaded and re-parsed every time. Measured in one 90-second window on a workspace with 32 jobs: 1980 CreateArtifactDownloadUrl completed 1230 "artifact is neither raw JSON nor a zip" parse failures 295 listBeamedRepos calls 217 CreateArtifactDownloadUrl AccessDeniedException That is ~22 download-URL creations per second, sustained. The AccessDenied errors were not confined to artifacts - ListMessages took the same error in the same window, so this starves unrelated calls, chat included. The existing bounds (BEAM_MAP_SCAN_MAX_CANDIDATES, BEAM_MAP_SCAN_BUDGET_MS) limit ONE scan and are correct, but they reset per call and so cannot limit the number of scans. Caching is what bounds the total. Artifacts are immutable - a re-upload mints a new artifactId - so a cached entry can never be stale. Keyed on artifactId, capped, and cleared wholesale on overflow rather than evicting a chosen victim; the only cost of a clear is downloading once more. A cached `null` means "these bytes are definitively not JSON", which is the case that dominated the waste (1230 of the failures). It also collapses that log line from once-per-sweep to once-per-artifact. Transient failures are deliberately NOT cached - a missing presigned URL, a network or throttle error, an AccessDenied, or a malformed zip that makes AdmZip throw. Caching one of those would permanently mark a readable artifact unreadable. Not changed here, deliberately: - No result-level cache for listBeamedRepos. Its result genuinely changes as lbvOpen flips, and making that staler risks the LBV-open timing that the Load gate depends on. With artifact downloads cached, a repeat call is a single ListArtifacts plus cache hits. - No name/extension prefilter on candidates. The code already documents why that regressed before: the beam-map is itself stored as a zip with a .zip path, so filtering on the extension skipped the beam-map entirely and fell back to zip-discovery with an empty stepId. - No cadence change on the IDE side. Measure the effect of this first.
… set The previous commit's cache did nothing. Built and run, the flood was unchanged at ~1200 CreateArtifactDownloadUrl per minute, and the new "caching as not-JSON" line fired 14649 times in 25 minutes. Measured why, rather than guessing again: distinct artifacts touched 1043 previous cap 500 wholesale clears in 25 min 36 The cap sat below the working set, so the cache filled, cleared everything, refilled, and cleared again - discarding on every pass the entries that were about to be hit. Hit rate approximately zero. It paid the bookkeeping and delivered none of the benefit. Two changes: 1. Cap 500 -> 4000, derived from the 1043 measurement rather than chosen. Redundancy in that session was ~29x (about 30000 downloads for 1043 distinct artifacts), so a cache that actually holds the working set should remove most of the volume. 2. Evict ONE least-recently-used entry instead of clearing the map, and refresh an entry's position on a hit so the ordering is a true LRU (Map iterates in insertion order, so "oldest key first" is only correct if hits re-insert). The eviction strategy is the part that mattered. Clearing wholesale turns an under-sized cap from a partial loss into a total one: at the cap it discards every entry including the ones in active use. Single-victim eviction degrades gradually, so a workspace whose working set exceeds the cap still gets most of the benefit instead of none. The cap-reached notice is kept but latched to log once per process rather than once per eviction - that signal is what made the under-sized cap diagnosable, and per-eviction logging would itself become a flood. Still to verify by measurement, not assertion: that this actually reduces the download volume. The previous version looked correct and did nothing.
The cache was keyed on artifactId with no expiry, justified by "a re-upload
mints a new artifactId". That is asserted nowhere in the platform contract,
and this file's own comments record the opposite: web-orc RE-WRITES the
beam-map ("re-written on each beam; latest wins"), and the BeamArtifactId
"thrash" that forced the beam-status fallback is the same observation. If a
rewrite reuses the id, a permanent cache pins the FIRST beam-map for the
whole LSP session — so a repo beamed later never appears in the panel until
restart, and a re-beam leaves chat routed at a dead stepId.
Split the policy by polarity instead, which needs no platform guarantee:
negatives ("not JSON") -> cached permanently; a property of the bytes,
and 1230 of the 1980 wasted fetches, so this
is where the win actually is
positives (parsed) -> 60s TTL; far longer than the ~13s sweep so the
repeated fetches still collapse, while
staleness is bounded to a minute
The TTL also bounds RETENTION: parsed values are arbitrary size, so without
one, customer artifact content stayed resident for the whole session.
Also label the beam discovery logs with their job id. One workspace commonly
holds several jobs containing the SAME repo names, so unlabelled per-repo
lines read as one repo changing state when they are really N jobs
interleaved — which produced a confident, wrong "lbvOpen is flapping"
diagnosis that was really four jobs each reporting correctly.
The skipPolling early return handed back the raw sendResult, whose sent-message
id sits at `message.messageId`. The polling path below it returns
`data: { sentMessage: sendResult.message }`, and the client reads
`data.sentMessage.messageId` to add the id to its seen-set so its own 3s poll
does not re-render the message it just sent locally.
On the skipPolling path that read resolved to null, nothing was seeded into the
seen-set, and the poll then treated the user's own message as newly arrived — so
every beamed message appeared twice in the transcript.
Return the same shape the polling path returns. Regression from the skipPolling
change; only beamed sends set the flag, so only they were affected.
Discovery matched a beamed repo by name and took the first hit. A repo beamed more than once has more than one node under that name, and the first is the oldest, so a re-beamed repo never came back - the IDE kept resolving to a step that had already finished. Matching now collects every node with that name and prefers one whose build-verification child is still live, falling back to the newest. A matched node's subtree is not searched again, so nested names cannot shadow their parent. The beam-map scan also had no reaction to being throttled: it would retry the same sweep on the next poll and keep the throttling alive. It now sits out a cooldown longer than the poll interval, so whole sweeps drop rather than being retried piecemeal. The cooldown empties the candidate list instead of returning early, because returning early would abandon beam-status discovery too and empty the tab for the whole cooldown.
…it forever A beam whose sub-agent never booted gets reaped by the web orchestrator: the instance is stopped and the repo node marked terminal. The IDE kept showing it, because the only signal read was the Local Build Verification child - and a reaped beam has none, which is indistinguishable from a fresh transfer whose HITL has not been created yet. Read the repo node's own status to separate them. Terminal with no LBV child means the beam ended without ever opening LBV, so it is closed. Safe on re-beam: the orchestrator re-drives that node to IN_PROGRESS before reusing it, so a re-beamed repo reads live again. The user is otherwise left waiting on a repo that will never build - the re-beam notice goes to the job owner, not to whoever is in the IDE.
The terminal-repo-node branch had no behavioural coverage. Deleting it or inverting it both left the suite green, so a passing run could not tell a working gate from an unreachable one. Four cases, mutation-verified: removing the gate fails two, over-gating fails the third, and the fourth pins that an LBV child still decides when one exists.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
|
/retrybuild |
pranav-firake
approved these changes
Sep 22, 2026
pranav-firake
approved these changes
Sep 22, 2026
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
3 participants
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.
What
LSP-side fixes for Beam to IDE (transfer a web-transformed repository into Visual Studio to run local build verification), found while verifying cross-user beamed chat end to end.
Seven commits across
netTransform/atxTransformHandler.tsand its tests:Scope — 30 of 33 hunks are beam-unreachable
ATXTransformHandleris large and shared, so every hunk was classified by call graph rather than by inspection:beam-prefixed, unreferenced off beamlistBeamedReposdownloadJsonArtifact+ cache (~97 added lines)listBeamedReposisRepoLbvOpen/pickLiveNode/findAllLbvNodeslistBeamedReposNon-beam behaviour changes — please read
Two hunks are not beam-gated. Both are fixes.
1. Throttle cooldown recorded in the shared retry wrapper. Any
ThrottlingExceptionnow records a cooldown timestamp. Only beam code reads it, so this is behaviourally inert off beam — but it is a write on the non-beam path that did not exist before.The reasoning is in the code and worth surfacing here: the existing per-call retry is resilience for one call and does not reduce our rate. Under sustained throttling it raises it, because a rejected call becomes up to four. Nothing told the client to stop starting work — which is the gap behind an alarm that has fired five times in 2026 without resolution, each round producing a local "make this cheaper" fix while the client kept initiating at the same rate. Skipping the beam-map scan is safe rather than a feature regression: the beam-map is optional enrichment, discovery already derives beamed repos from beam-status artifacts when it is absent, and
isRepoLbvOpendefaults to OPEN on a fetch failure, so nothing is ever wrongly hidden.2.
sendMessagenon-polling return shape. That path returned the raw send result, which nests the sent message id undermessagerather thansentMessage. The client readsdata.sentMessage.messageIdto add it to its seen-set, so with the id unreadable every message on that path rendered twice — once locally, once again from the poll. The shape now matches the polling path.This path triggers on
!sentMessageId || skipPolling. Beam setsskipPolling, so beam hit it directly;!sentMessageIdcan occur on any send, so the fix reaches non-beam too.No other consumer is affected.
ATXTransformHandleris registered only byatxNetTransformServer.ts, and the ATX .NET transform is a Visual Studio feature — VS Code and JetBrains never instantiate that server, so neither change can reach them.Verification
tsc --noEmitclean, rebased onto currentmainwith zero conflictsNot included
An unwired beam handoff-artifact consumer is deliberately excluded and will land with its producer wiring rather than as an unused method.