Skip to content

fix(server): keep tool payloads out of completion queues - #12345

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
automation/perf-audit-20260917-191305
Sep 18, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
automation/perf-audit-20260917-191305

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

The terminal-run worker can wait on a provider or a thread lock while its event subscription queues every raw event. Filtering after that queue lets unrelated tool output stay in memory for the entire wait.

Filter subscriptions by event type before live publication and apply the same filter in SQL before decoding replay payloads. The completion worker subscribes to run updates, preserving their order and delivery. This is shared server behavior for every client and provider; the wire protocol is unchanged.

Based on #2829.

Before and after

A GC-checked reproduction using the application EventSink blocks the consumer on a run update, then publishes 32 unrelated events containing 16 MiB of synthetic payloads.

Measurement Before After
Unrelated payload objects retained 32 0
Heap used in the reproduction 65 MiB 49 MiB

The new regression fails on the original implementation and passes with the fix. It verifies filtered replay, skipping unreadable unrelated payloads before decoding, and ordered live delivery through an output burst.

Validation

  • 50 focused tests passed across foundation persistence, event-store sequence queries, event-store persistence, and provider restart recovery.
  • Server package typecheck, scoped lint, formatting, and diff checks passed. Lint retains the existing unused layerUnavailable warning.
  • Screenshots do not apply to this server buffering change. The behavior and memory evidence are above.

Follow-up memory audit

Three more retention defects were reproduced and fixed in separate local commits. These follow-up fixes are not included in this PR's current head:

Retention path Fix Local commit
Unused Codex raw observation buffers keep notifications after normal handlers finish Disable raw capture in T3 runtimes and probes while preserving full handler payloads and the library default 6b989c7bd3a
Shared provider sessions buffer another run's output before checking ownership Filter run ownership before enqueueing, preserving subagent discovery and carryover completions during recovery 7a1e84a0098
Closing a subscription after provider termination leaves its queued payloads retained Clear closed queues even when termination has already removed their registry entry 7a1e84a0098

Each separate GC-checked reproduction retained 32 synthetic payload objects totaling 16 MiB before its fix and zero afterward. The follow-up commits passed 312 focused tests, server and transport package typechecks, and scoped lint and formatting checks. They are on the local branch automation/provider-memory-20260917-193938 and still need publication and integration.

This fixes a reproduced retention defect. The available backend heap-exhaustion report does not prove it was the cause of that production crash. Integrate the change and rebuild the server or desktop bundle to apply it.

Prepared with Codex.

Filter terminal-worker subscriptions before live buffering and historical payload decoding. Preserve matching run updates without retaining unrelated tool output while queue promotion waits on providers or thread locks.
@juliusmarminge
juliusmarminge added this pull request to stack #12346 September 18, 2026 02:19
@juliusmarminge juliusmarminge changed the title automation/perf audit 20260917 191305 fix(server): keep tool payloads out of completion queues Sep 18, 2026
@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 18, 2026
@coderabbitai

coderabbitai Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: f082762c-1f4a-4691-93a5-84cb9ef95b65

📥 Commits

Reviewing files that changed from the base of the PR and between ebfd663 and a56ae77.

📒 Files selected for processing (6)
  • apps/server/src/orchestration-v2/EventSink.ts
  • apps/server/src/orchestration-v2/EventStore.ts
  • apps/server/src/orchestration-v2/FoundationPersistence.test.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/persistence/Layers/OrchestrationEventStore.ts
  • apps/server/src/persistence/Services/OrchestrationEventStore.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Changes

The orchestration event APIs now accept optional event type filters. Persistence applies the filter to replay queries. Event sinks apply it to replay and live streams through type-specific PubSubs. Startup monitoring and persistence tests use the filtered behavior.

Event type filtering

Layer / File(s) Summary
Persistence event type filtering
apps/server/src/persistence/Services/OrchestrationEventStore.ts, apps/server/src/persistence/Layers/OrchestrationEventStore.ts, apps/server/src/orchestration-v2/EventStore.ts
Event read contracts accept eventType. Application event queries add an event_type predicate when the filter is present.
Filtered replay and live streaming
apps/server/src/orchestration-v2/EventSink.ts
Streams forward event type filters to replay reads and use type-specific PubSubs for live events. Event writers publish committed events to the global and matching type-specific channels.
Monitoring and validation
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/FoundationPersistence.test.ts
Startup monitoring subscribes only to run.updated events. Tests cover thread and event type filtering for replayed and live events.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Orchestrator
  participant EventSinkV2
  participant EventStoreV2
  participant OrchestrationEventStore
  participant PubSub
  Orchestrator->>EventSinkV2: subscribe to run.updated
  EventSinkV2->>EventStoreV2: read replay with eventType
  EventStoreV2->>OrchestrationEventStore: query matching event_type
  OrchestrationEventStore-->>EventSinkV2: return matching historical events
  EventSinkV2->>PubSub: subscribe to run.updated channel
  PubSub-->>EventSinkV2: publish matching live events
  EventSinkV2-->>Orchestrator: deliver filtered stream
Loading

Suggested reviewers: mwolson, saphid

Merge Risk: ⚪ Minimal · up to a56ae

The stream filtering change has no identified actionable regression and is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the primary change: preventing tool payloads from remaining in completion queues.
Description check ✅ Passed The description explains the problem, the filtering approach, validation results, memory impact, and why UI evidence does not apply. It does not use the template headings or checklist format, but it c…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

⚠️ The latest CI run did not produce a thread transfer result for a56ae77.

This comment will update automatically after the next completed run.

@macroscopeapp

This comment has been minimized.

@macroscopeapp

macroscopeapp Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at a56ae77

Macroscope's review found this PR approvable — This is a focused orchestration bug fix that filters run-update events before live queue buffering and historical decoding, while preserving existing unfiltered streams and stored-event behavior. The targeted persistence and load-oriented tests cover the changed path without introducing schema, deployment, default, or static-analysis changes.

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

@juliusmarminge
juliusmarminge removed this pull request from stack #12346 September 18, 2026 02:52
@juliusmarminge
juliusmarminge merged commit 727c2e0 into t3code/codex-turn-mapping Sep 18, 2026
22 checks passed
@juliusmarminge
juliusmarminge deleted the automation/perf-audit-20260917-191305 branch September 18, 2026 02:56
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