Read the agent status through its Arc instead of cloning the record - #3829
Draft
kmatasfp wants to merge 1 commit into
Draft
Read the agent status through its Arc instead of cloning the record#3829kmatasfp wants to merge 1 commit into
kmatasfp wants to merge 1 commit into
Conversation
✅ Deploy Preview for golemcloud canceled.
|
kmatasfp
marked this pull request as draft
September 4, 2026 09:53
Contributor
Author
|
Testing similar change out against 1.5.x first |
kmatasfp
force-pushed
the
gol-539-invocation-results
branch
from
September 4, 2026 10:08
7270a21 to
a6da4a3
Compare
|
Found 5 test failures on Blacksmith runners: Failures
|
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.
Reading one field out of
AgentStatusRecordcurrently deep-copies the wholerecord, and the record owns
invocation_results: a map that gains an entry forevery invocation a worker has accepted and is never pruned. The clone therefore
copies the worker's entire invocation history, and the hottest of the sites that
does it is
lookup_invocation_result, which runs once per invocation. Cost perinvocation grows with the number of invocations already served, so the total is
quadratic in a long-lived worker's invocation count.
last_known_statusis anArcSwap, andload_full()already returns aconsistent snapshot with the lifetime every one of these callers needs. Each of
them only reads. So the fix is to keep the
Arcinstead of cloning out of it —seven sites in
worker/mod.rsplus theNonDetachedStatusjob instate_actor.rs, which now matches theAttachedStatusjob sitting next to itthat already returned an
Arc.Three call sites moved a small field out of the returned value and now clone that
field instead:
pending_card_eventstwice andcurrent_idempotency_keyonce.None of them is the growing map.
Nothing about what is read changes, so agent behaviour is unaffected. The
Arc<AgentStatusRecord>return type is also a small guard against the samemistake coming back, since a future
.clone()on it is visible at the call site.How it showed up
A chaos run driving 800 ops/s across ~200 agents for 12 minutes showed executor
CPU climbing steadily from start to finish and RSS growing over the run. A run of
the same shape at 100 ops/s — same agents, same duration, an eighth of the
invocations per agent — stayed flat, which is the signature of a per-invocation
cost that scales with invocation count rather than with agent count or wall time.
What this does not fix
The CPU half only. Two things still grow without bound for the life of a worker,
and neither is addressed here:
AgentStatusRecord::invocation_resultskeeps one(IdempotencyKey, OplogIndex)entry per invocation forever. Pruning it would change what arepeated idempotency key resolves to, so it needs its own design.
Worker::invocation_resultscache stores each successfulinvocation's full
AgentInvocationOutputand nothing evicts it. This is thelarger of the two and is the likelier source of the observed RSS growth.
Eviction looks safe in principle — a missing entry falls back to
InvocationResult::Lazyand re-reads the result from the oplog — but that is abehaviour change and a separate piece of work.
So a build carrying this commit should show flat executor CPU across a long run
and still show memory growing.
Testing
cargo check --workspace --all-targetsclean;cargo test -p golem-worker-executor --lib1568 passed, 0 failed.