Skip to content

perf(node): avoid per-message DataType clone in first-message type check - #2838

Merged
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-0efkxi-node-type-check-dedup
Jul 28, 2026
Merged

perf(node): avoid per-message DataType clone in first-message type check#2838
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-0efkxi-node-type-check-dedup

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

The first-message input type check ran on both receive paths (recv_async and Stream::poll_next) as two copy-pasted blocks, each doing self.input_type_checks.get(id).cloned() before deciding whether the message was pattern-correlated. For service/action inputs the check stays armed indefinitely (those inputs are polymorphic by pattern design), so the stored DataType — which can be a nested Struct / List — was cloned and then discarded on every message on that input.

The two blocks were also byte-for-byte duplicates (differing only in a log-message suffix), a drift risk the surrounding adora#172/#174 comments explicitly call out.

Fix

Consolidate both paths into one note_produced_event helper and order the guard so it is cheap in every case:

  1. input_type_checks.contains_key(id) — a single map lookup that short-circuits once the check has been consumed, preserving the existing "zero extra cost after the first message per input" property for steady-state topic traffic.
  2. carries_pattern_correlation(...) — the shared predicate (replacing the inlined REQUEST_ID/GOAL_ID/GOAL_STATUS triple-check), evaluated only while the check is still armed.
  3. remove(id) — the value is taken, not cloned, and only for a non-pattern message.

Net effect: the DataType clone is removed entirely; the common consumed-check path stays at one lookup; and the two previously drift-prone blocks now share a single method, so recv() and StreamExt::next() cannot diverge.

Note: an earlier iteration of this change put the pattern predicate first, which would have added 3 BTreeMap lookups to every steady-state topic message. The contains_key-first ordering avoids that regression — it is strictly cheaper than the original on the common path and avoids the clone on the armed path.

Validation

  • cargo clippy -p dora-node-api --lib -- -D warnings clean (and --all-targets).
  • cargo test -p dora-node-api --lib → 147 passed.
  • Predicate parity with the old inline check is covered by the existing carries_pattern_correlation_* unit tests.

⚠️ This PR is machine-generated by Claude (an AI assistant) as part of an automated code-review run, and was verified against current origin/main. Please review carefully before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TMv79fayzGF9mDV7vPMQN7


Generated by Claude Code

The first-message input type check ran on both receive paths
(`recv_async` and `Stream::poll_next`) as two copy-pasted blocks that
each did `self.input_type_checks.get(id).cloned()` *before* deciding
whether the message was pattern-correlated. For service/action inputs the
check stays armed indefinitely (those inputs are polymorphic by pattern
design), so the stored `DataType` — which can be a nested
`Struct`/`List` — was cloned and then discarded on *every* message on
that input.

Consolidate both paths into one `note_produced_event` helper and order
the guard so it is cheap in every case:

1. `input_type_checks.contains_key(id)` — a single map lookup that
   short-circuits once the check has been consumed, preserving the
   existing "zero extra cost after the first message per input" property
   for steady-state topic traffic.
2. `carries_pattern_correlation(...)` — the shared predicate (replacing
   the inlined `REQUEST_ID`/`GOAL_ID`/`GOAL_STATUS` triple-check),
   evaluated only while the check is still armed.
3. `remove(id)` — taken (not cloned) only for a non-pattern message.

This removes the `DataType` clone entirely, keeps the common consumed-
check path at one lookup, and unifies the two previously drift-prone
blocks (which the surrounding adora#172/#174 comments call out) behind a
single method, so `recv()` and `StreamExt::next()` cannot diverge.

This change is machine-generated by Claude (an AI assistant) as part of an
automated code-review run, and reviewed against current origin/main.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TMv79fayzGF9mDV7vPMQN7
@trunk-io

trunk-io Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — fully automated, no human in the loop.

The shared carries_pattern_correlation (libraries/message/src/metadata.rs:200-204) tests exactly the same three keys (REQUEST_ID, GOAL_ID, GOAL_STATUS) as the inlined triple-check it replaces, and the new guard order — contains_key(id)!carries_pattern_correlation(...)remove(id) — is behavior-equivalent while eliminating the per-message DataType clone for still-armed pattern inputs. Unifying both receive paths behind one helper avoids future drift. No issues found.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review July 28, 2026 12:28
@trunk-io
trunk-io Bot merged commit 189ed77 into main Jul 28, 2026
27 checks passed
@trunk-io
trunk-io Bot deleted the claude/dreamy-bardeen-0efkxi-node-type-check-dedup branch July 28, 2026 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants