fix(runtime): retired sessions start fresh after restart - #522
Draft
vincentkoc wants to merge 1 commit into
Draft
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 26, 2026, 11:25 PM ET / August 27, 2026, 03:25 UTC. ClawSweeper reviewWhat this changesThe PR adds an optional runtime method that locally retires an exact persisted ACPX session record so the next ensure creates a new backend session after restart. Merge readinessKeep open for explicit maintainer acceptance of a new public runtime contract. The implementation is coherent and targets ACPX’s backend session-lifecycle boundary; the linked OpenClaw work supplies motivation but is not an ACPX implementation dependency. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — step 1 Assertions:
How this fits togetherACPX’s embedded runtime maps host session handles to persisted records and retained ACP client connections. This change retires one exact record, then makes the next session ensure create a fresh backend session. flowchart LR
Host[Embedding host] --> Handle[Exact session handle]
Handle --> Runtime[ACPX runtime]
Runtime --> Manager[Session manager]
Manager --> Owner[Active or retained owner]
Manager --> Store[Persisted session record]
Store --> Ensure[Next session ensure]
Ensure --> Backend[Fresh backend session]
Decision needed
Why: The patch is technically focused, but it adds a lasting public API convention and the repository policy requires deliberate scrutiny of new runtime surface. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt one narrowly documented, opt-in local-retirement operation if maintainers want embedding hosts to reset a record without requiring ACP backend session-close support. Do we have a high-confidence way to reproduce the issue? Yes, source reproducibly shows that current main’s reset marker forces a fresh ensure, while the new committed three-process harness exercises durable local retirement across restart boundaries; this read-only review did not execute it. Is this the best way to solve the issue? Yes, subject to contract approval: an explicit opt-in exact-handle operation is narrower than changing existing close semantics or inferring identity from a session key. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4c16ab32154. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: openclaw/openclaw#130601
Related: openclaw/openclaw#107487
What Problem This Solves
Fixes an issue where an embedding host could retire an ACPX session locally, restart, and then accidentally resume the same backend session because the exact ACPX record was never durably marked for reset.
Credit to @MoerAI for the original restart-boundary report and patch direction in OpenClaw #130601.
Why This Change Was Made
This adds the upstream ACPX runtime operation
prepareFreshSession(handle). It useshandle.acpxRecordId ?? handle.sessionKey, so generated one-shot records and persistent records are retired by exact identity rather than inferring a one-shot record from a bare session key.The manager now:
closed: trueplusacpx.reset_on_next_ensure: trueonly after owner finalization succeeds;close()/requireRecord()contract;session/closefor this operation.No ACP protocol, persistence schema, configuration, sidecar, or fallback path changes are included. No OpenClaw code is changed in this PR.
User Impact
Embedding hosts can durably retire an exact ACPX persistent or generated one-shot record. After the operation resolves, the next ensure creates a fresh backend session even after a process restart.
OpenClaw integration remains pending until this ACPX change is merged and published in a new ACPX version; OpenClaw #130601 must then consume that version and pass the exact runtime handle.
Evidence
Exact tested head:
865a79ec00f81946421c72b87326a8d6a6b2b379Tests and Gates
pnpm run build:testfailed withTS2339becauseprepareFreshSessiondid not exist.pnpm run check: passed.pnpm run check:mutation: passed at 90.80% (385 killed, 39 existing survivors in unchangedsrc/cli/flags.ts).{ sessionKey }API-shape finding was not applied because this upstream PR intentionally establishes the reviewed exact-handle contract required for one-shot identity. The provisional downstream OpenClaw wrapper must adapt after publication.Manager coverage includes:
FileSessionStorepermission failure without false retirement;Three-Process FileSessionStore Proof
The committed test harness runs seed, retire, and ensure in separate OS processes over one real temporary
FileSessionStore.The third process receives a different backend session ID while retaining the exact ACPX record identity.
Codex Restart Contract
Personally inspected sibling
openai/codexata6e63f9f32525d171676ac49c4e87ab00d76f0ce:codex-rs/app-server-protocol/src/protocol/v2/thread.rs:321-336defines resume by stored thread ID.codex-rs/app-server/src/message_processor.rs:1128-1148dispatches thread resume.codex-rs/app-server/src/request_processors/thread_processor.rs:3542-3650reloads stored thread state during cold resume.This confirms that leaving the stale backend ID reusable can resume it after a host restart.
LOC
+54/-6(+48net).+390/-0.+2/-0.The positive production delta is the new public runtime capability plus the lifecycle barrier and strict failure-propagating owner finalizer. The compression pass kept the existing reset marker and close machinery, with no duplicate storage or protocol path.
Residual Risk
prepareFreshSession()waits for active-turn finalization. If an ACP agent ignores cancellation, completion is bounded by the existing turn timeout/finalization behavior rather than falsely acknowledging retirement.AI-assisted: yes. The change was implemented test-first, independently reviewed, and validated against the real file store and OS process boundary.