Skip to content

perf(server): ACP item ordinals no longer copy a session-wide map per item - #13868

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/acp-item-ordinals
Sep 26, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/acp-item-ordinals

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

The ACP V2 adapter (used by Grok, Devin, Antigravity and the registry agents) has the same item-ordinal problem as Claude (#13864) and Cursor (#13866). Every item ordinal of the session sat in one Map that was copied on each new item and never pruned. Only a rollback's runtime replacement cleared it. A long session paid O(items) memory and O(items²) copying.

What changed

  • Root items: the ordinal map moves onto the turn (ActiveAcpTurn.itemOrdinals). It is mutated in place and goes away when the turn settles. The per-turn counter map is gone, because the map's size is the counter.
  • Subagent child items (a child session's messages and tool calls) used the same session map as a dedupe cache, while their ordinals came from the subagent's own nextChildOrdinal. They now keep that cache on the subagent (childItemOrdinals). The subagent record already carries over into later turns (carryoverSubagents), so a child update that arrives after the root turn settled still finds its ordinal.
  • The rollback reset of the two session maps is gone, because the maps are gone. Rollback already refuses while a turn is active.

Why per-turn is safe for root items

Every resolveItemOrdinal call passes the active turn. A subagent keeps its own turnItemOrdinal on its record (existing?.turnItemOrdinal ?? resolveItemOrdinal(...)), including across carryover. Before the fix I instrumented resolveItemOrdinal to flag any hit on an ordinal a different turn had allocated, then ran all replay suites plus the ACP, registry and Grok adapter tests: 260 ACP allocations, 3 cross-turn hits. All 3 came from the mock ACP agent in AcpAdapterV2.test.ts ("waits for immediate allow and deny permission responses…"), which hardcodes toolCallId: "tool-call-1" on every prompt. A real agent does not reuse a tool call id across prompts. Even for that mock, the only effect of the fix is that turn 2's tool gets a turn-2 ordinal instead of reusing turn 1's. The test does not assert ordinals, and it passes unchanged.

Benchmark

This is the same resolver shape as Claude and Cursor. A scratch, uncommitted script ran it before and after, in 10 turns, resolving each item twice:

N items before after
1,000 60 ms 1 ms
10,000 7.6 s 4 ms
50,000 253 s 37 ms

A 50k-entry map retains about 1.8 MiB for the life of the session. After the fix, root-item memory is bounded by the largest single turn, and child-item memory by the subagent's lifetime.

Verification

  • vp test run with AcpAdapterV2.test.ts, AcpRegistryAdapterV2.test.ts, GrokAdapterV2.test.ts, AntigravityAdapterV2.test.ts, OrchestratorReplayFixtures.integration.test.ts (which includes the Grok and registry replay fixtures: background bash, background subagent, monitor, subagent lineage) and OrchestratorReplayRecovery.integration.test.ts: 6 files, 244 tests pass. The replay fixtures are unchanged.
  • vp exec tsc --noEmit -p . in apps/server: no errors.
  • vp lint on the touched file: only the existing unused NodePath import warning.
  • Not run: repo-wide checks, live ACP agents.

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code


Devin Review

… item

The ACP V2 adapter (Grok, Devin, Antigravity and registry agents) kept
every item ordinal of the session in one Map that it copied on each new
item and never pruned, so a long session paid O(items) memory and
O(items^2) copying. Root item ordinals are only looked up by the turn that
allocated them, so that map now lives on the turn. Subagent child items
used the same map with ordinals from the subagent's own counter; they now
keep them on the subagent, which already carries over into later turns.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 26, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 9d8ca68

Macroscope's review found this PR approvable — This is a localized ACP adapter optimization that bounds ordinal-map lifetime and removes quadratic map copying while preserving per-turn and per-subagent ordinal allocation. It introduces no new capability, schema, product-default, security, billing, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

return updated;
const resolveItemOrdinal = (context: ActiveAcpTurn, nativeItemId: string) =>
Effect.sync(() => {
const existing = context.itemOrdinals.get(nativeItemId);

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.

Scoping ordinals to each turn changes the result when a native item ID recurs in a later turn: it now receives that turn's ordinal instead of the earlier turn's. Could you add a focused adapter regression test covering reused IDs across turns and stable ordinals on repeated updates? This backend behavior change needs test coverage.

Posted via Macroscope — Effect Service Conventions

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 4.9 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 4.9 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 20.8 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: 9d8ca68 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@juliusmarminge
juliusmarminge merged commit 52d84bd into t3code/codex-turn-mapping Sep 26, 2026
24 of 25 checks passed
@juliusmarminge
juliusmarminge deleted the v2/acp-item-ordinals branch September 26, 2026 22:02
dillonc-dev added a commit to exarch-run/t3code that referenced this pull request Sep 27, 2026
… item (pingdotgg#13868)

(cherry picked from commit 52d84bd)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant