Skip to content

Stop cloning the agent status record on the invocation path - #3830

Merged
kmatasfp merged 2 commits into
1.5.xfrom
gol-539-15x
Sep 5, 2026
Merged

Stop cloning the agent status record on the invocation path#3830
kmatasfp merged 2 commits into
1.5.xfrom
gol-539-15x

Conversation

@kmatasfp

@kmatasfp kmatasfp commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

AgentStatusRecord owns invocation_results, a map that gains an entry for
every invocation an agent has ever served and is never pruned. Anything that
clones the record therefore costs more on every invocation, and the executor was
cloning it roughly a dozen times per invocation:

  • commit_and_update_state_inner runs on every oplog commit (two or three per
    invocation, more with durable host calls) and copied the record three times,
    then update_last_known_status copied it a fourth time and the != check
    walked the whole map.
  • drain_pending_from_status copied it two or three times per invocation to
    read pending_updates.front(), pending_invocations.first() and one
    timestamp.
  • The gRPC invoke path copied it once per invocation through
    get_latest_metadata, to check the agent had not failed.
  • The oplog-processor ForwardingOplog, which wraps every agent's oplog
    whether or not it has plugins, copied it every third commit
    (plugin_max_commit_count) in try_flush, and again on its timer.
  • checkpoint_status_mid_invocation copied it after every durable function,
    before the throttle that discards nearly all of those calls.
  • lookup_invocation_result copied it once per invocation (the first commit on
    this branch).

The cost of serving an invocation grew with the number already served: linear
per invocation, quadratic over the agent's life, while throughput stayed flat.
That is the creep.

Measured

A throwaway in-process test (one durable Counter agent, increment invoked
20,000 times sequentially, debug build, Redis-backed) with perf stat counting
retired user-space instructions of the executor process per window of 1,000:

history before window unfixed, M instr/inv fixed, M instr/inv
0 15.7 7.3
5,000 95.9 7.7
10,000 171.4 8.1
15,000 255.0 8.3
19,000 316.9 8.9
fresh agent, same process, after the 20,000 6.8

Retired instructions rather than CPU seconds because the machine is shared;
CPU per invocation tells the same story (6.4ms rising to 23.7ms unfixed, flat at
3.4 to 4.2ms fixed). Every intermediate step was measured the same way: the
first three sites above took the slope from 15.8M to 1.65M per 1,000 of
history, the forwarding oplog halved that, and the invoke path removed the rest.
The fresh-agent control is what showed the remainder was per-agent history and
not process state.

perf record on the unfixed build late in the run put about 70% of user CPU in
malloc, free and hashbrown's clone, iterate and drop of the
(IdempotencyKey, OplogIndex) map.

What changed

  • The commit path takes the record out of its lock, folds the new entries onto
    it and puts the result back, copying nothing. update_status_with_new_entries
    gained a Result twin, fold_status_with_new_entries, that hands the record
    back on the detach path so the lock never holds a placeholder. The != check
    is replaced by "were there any new entries": the fold advances oplog_idx on
    any entry and is the identity on none, so the two are equivalent.
  • get_non_detached_last_known_status is now
    with_non_detached_last_known_status(|status| ...): callers read the field or
    two they need under the guard. Every caller was converted; none needed more
    than a scalar, a PendingInvocationRef or the (small) current_retry_state.
  • StatusCheckpointer::maybe_checkpoint reads the live status through its lock
    and copies it only once it has decided to write, and the get_oplog_index
    marker guard moved in with it. Regression test added for the marker.
  • ForwardingOplogState reconciles plugin state from active_plugins and
    oplog_processor_checkpoints alone, and copies the record only when there is
    a plugin to send to.
  • ensure_not_failed takes the agent status and deleted_regions, which is
    all last_error reads, so WorkerCtx::get_last_error_and_retry_count takes
    &DeletedRegions instead of the record. The invoke path reads both under the
    resident worker's status lock; a non-resident worker still goes through
    get_latest_metadata as before.
  • The flusher's on_status_changed takes the previous record's tracking bit
    rather than the record, and WorkerService::set_assignment_tracking takes
    that bit rather than a record. The ephemeral guard moved to the two callers.
    Recovery-index writes fire on exactly the same transitions as before; the
    flusher test now also asserts the direction of each write.

No ordering changes: the write lock is held only for the synchronous fold,
the recovery-index write and dirty-marking happen after the new record is
installed, as before.

What this does not fix

  • AgentStatusRecord::invocation_results and the in-memory
    Worker::invocation_results cache still retain one entry per invocation for
    the life of the agent, so RSS still grows with invocations served. Bounding
    them is a retention-policy change (Bound invocation result state in worker status #3820 on main).
  • The background status flusher still copies the record once per second per
    dirty agent and walks the map to compute its delta. That is proportional to
    resident history rather than to invocation rate, and small at the history
    sizes the chaos runs reach; it stops being small for agents with hundreds of
    thousands of invocations. Same root cause as the retention point above.

Testing

cargo test -p golem-worker-executor --lib 535 passed; cargo clippy --workspace --all-targets --no-deps -- -D warnings and cargo check --workspace --all-targets
clean.

@blacksmith-sh

This comment has been minimized.

vigoo commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

This is probably going to be conflicting with #3820 when ported to main

@kmatasfp kmatasfp changed the title Read the fields an invocation lookup needs instead of cloning the status record Stop cloning the agent status record on the invocation path Sep 4, 2026
@kmatasfp
kmatasfp merged commit 98eb965 into 1.5.x Sep 5, 2026
96 of 102 checks passed
@kmatasfp
kmatasfp deleted the gol-539-15x branch September 5, 2026 07:21
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants