fix(gateway): ignore stale abort markers for fresh chat events#91013
Conversation
|
Codex review: passed. Reviewed June 19, 2026, 2:29 AM ET / 06:29 UTC. Summary PR surface: Source +79, Tests +103. Total +182 across 13 files. Reproducibility: yes. source-level: on current main, seed abortedRuns for a client run id, register a same-key fresh run, then send assistant delta and lifecycle end; the presence-only checks suppress both projections. I did not execute tests in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the ordered abort-marker freshness fix through the maintainer-authorized automerge path after exact-head checks and mergeability gates pass, keeping chat.send replay semantics unchanged unless maintainers request broader behavior. Do we have a high-confidence way to reproduce the issue? Yes, source-level: on current main, seed abortedRuns for a client run id, register a same-key fresh run, then send assistant delta and lifecycle end; the presence-only checks suppress both projections. I did not execute tests in this read-only review. Is this the best way to solve the issue? Yes. The PR puts ordering metadata in the Gateway chat-state owner and applies the same helper at both projection checks; keeping chat.send replay unchanged is acceptable once maintainers accept that scope. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5d6ac2308657. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +79, Tests +103. Total +182 across 13 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Resolve conflicts from unrelated upstream churn (control-ui first-assistant tracing, telegram): - server-chat-state.ts: keep upstream ChatRunTiming (incl firstAssistantEventSent) and chatSendTiming; name the base struct ChatRunRegistration so the derived ChatRunEntry freshness type still applies. - server-methods/chat.ts: union the same-module imports (chatAbortMarkerTimestampMs value + ChatRunTiming type) into one statement. No fix logic changed; re-anchors merge-base to current main.
…20260614T022510 # Conflicts: # src/gateway/server-chat.agent-events.test.ts # src/gateway/server-chat.ts
|
/clownfish automerge |
|
Clownfish is on the reef for this PR. 🐠 I tagged A maintainer can call |
|
🦞✅ Source: What merged:
Automerge notes:
The automerge loop is complete. Automerge progress:
|
…law#91013) Summary: - The branch stamps Gateway chat run registrations and abort markers with ordering metadata, uses freshness checks for chat projection suppression, and updates abort/restart/maintenance tests and related types. - PR surface: Source +79, Tests +103. Total +182 across 13 files. - Reproducibility: yes. source-level: on current main, seed abortedRuns for a client run id, register a same-k ... end; the presence-only checks suppress both projections. I did not execute tests in this read-only review. Automerge notes: - PR branch already contained follow-up commit before automerge: ci: re-trigger checks against current main - PR branch already contained follow-up commit before automerge: Merge upstream/main into stale-abort marker fix - PR branch already contained follow-up commit before automerge: Merge remote-tracking branch 'upstream/main' into nex/91013-conflict-… Validation: - ClawSweeper review passed for head 6f13d6f. - Required merge gates passed before the squash merge. Prepared head SHA: 6f13d6f Review: openclaw#91013 (comment) Co-authored-by: nxmxbbd <[email protected]>
…law#91013) Summary: - The branch stamps Gateway chat run registrations and abort markers with ordering metadata, uses freshness checks for chat projection suppression, and updates abort/restart/maintenance tests and related types. - PR surface: Source +79, Tests +103. Total +182 across 13 files. - Reproducibility: yes. source-level: on current main, seed abortedRuns for a client run id, register a same-k ... end; the presence-only checks suppress both projections. I did not execute tests in this read-only review. Automerge notes: - PR branch already contained follow-up commit before automerge: ci: re-trigger checks against current main - PR branch already contained follow-up commit before automerge: Merge upstream/main into stale-abort marker fix - PR branch already contained follow-up commit before automerge: Merge remote-tracking branch 'upstream/main' into nex/91013-conflict-… Validation: - ClawSweeper review passed for head 6f13d6f. - Required merge gates passed before the squash merge. Prepared head SHA: 6f13d6f Review: openclaw#91013 (comment) Co-authored-by: nxmxbbd <[email protected]>
Summary
What problem does this PR solve?
Why does this matter now?
What is the intended outcome?
{ abortedAtMs, sequence }form as normal abort markers.What is intentionally out of scope?
What does success look like?
deltaandfinalwhen the only abort marker is older than the fresh registration.What should reviewers focus on?
ChatAbortMarkerfreshness ordering insrc/gateway/server-chat-state.ts.src/gateway/server-chat.tsthat compare markers against the current chat run link.src/gateway/server-close.tsusing the same marker factory.Linked context
Which issue does this close?
Fixes #91012
Which issues, PRs, or discussions are related?
Related #84176
Was this requested by a maintainer or owner?
No direct maintainer request; this is a focused follow-up to the current Gateway abort/send behavior.
Real behavior proof (required for external PRs)
Stale chat abort markers should not suppress events for a fresh chat run registered after that marker, while a current abort marker should still suppress the active run.
Local OpenClaw checkout on this branch, importing the production Gateway chat runtime from
src/gateway/server-chat.tswithtsxand isolated config/state.From the repository root, a standalone Node runtime harness imported
src/gateway/server-chat.ts, created realcreateChatRunState()/createAgentEventHandler()instances, and drove two cases:Terminal capture from this branch, copied live output:
The stale marker case emitted both
deltaandfinalfor chat and node subscribers, retained the marker for TTL cleanup, and cleared the registry. The current marker case emitted no chat/node states, retained the marker, and cleared the registry.External provider/network execution and browser/native client rendering were not exercised; this change is limited to Gateway chat abort marker freshness and event projection.
The runtime proof is a local production-source harness rather than a hosted end-to-end provider run. Focused regression tests and type/lint/format gates are included below as supplemental coverage.
Current-main RED proof for the stale marker case failed with no fresh chat states emitted:
Tests and validation
Which commands did you run?
What regression coverage was added or updated?
What failed before this fix, if known?
mainbecause no fresh chat states were emitted.If no test was added, why not?
N/A --- focused regression coverage was added.
Risk checklist
Did user-visible behavior change? (
Yes/No)Yes. Fresh chat events are no longer hidden by stale abort markers.
Did config, environment, or migration behavior change? (
Yes/No)No.
Did security, auth, secrets, network, or tool execution behavior change? (
Yes/No)No.
What is the highest-risk area?
Cancellation/lifecycle idempotency around
clientRunIdand provider eventrunIdaliases.How is that risk mitigated?
The patch stamps chat run registrations and abort markers through one process-local ordering sequence, checks both aliases against the current chat link, keeps legacy numeric marker timestamp fallback, and preserves TTL timestamp extraction for maintenance/retry paths.
Current review state
What is the next action?
Maintainer review and normal PR CI after the branch update.
What is still waiting on author, maintainer, CI, or external proof?
Nothing known from the author side; local conflict repair and focused validation are complete.
Which bot or reviewer comments were addressed?
Existing automated review requested maintainer review; no author-side blocker remains known.