Skip to content

feat(acp): restore Antigravity sessions after restart - #5670

Open
Sun-GLiang wants to merge 5 commits into
apache:mainfrom
Sun-GLiang:codex/acp-session-restore
Open

Sun-GLiang wants to merge 5 commits into
apache:mainfrom
Sun-GLiang:codex/acp-session-restore

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

Maka could read an Antigravity task after its ACP process or Host restarted, but could not continue the same external Session. This PR persists the external Session ID and a versioned prompt checkpoint in Plugin-private storage, then restores a committed task through the Agent's negotiated session/resume method. The original Session and saved model are retained; no earlier prompt is resent and restoration never falls back to session/new.

A completed turn becomes committed only after Maka has consumed its canonical terminal event. If a prompt may have progressed beyond saved history, session/load replay is kept outside the canonical conversation and the task shows an explicit history gap. Desktop now shows restorable, restoring, restore-failed, and history-gap states with Restore, retry, or New Task actions as appropriate. Tasks created before this change remain history-only because they have no saved external Session ID.

Refs #5103

Verification

  • npm run build, npm run typecheck, npm run lint, and npm run format:check passed.
  • npm run check:renderer-architecture, npm run check:locale-hygiene, and the protocol compatibility epoch guard passed. The epoch advances from 189 to 190.
  • All workspace suites passed with node scripts/run-workspace-tests-parallel.mjs --concurrency=1 on bd8661f3a. The branch then rebased to b62ca805e; the final build, typecheck, lint, format, epoch guard, and 104 affected tests passed after that rebase.
  • Controlled real-stdio tests cover a forcibly killed process after a durable acknowledgement, same-Session continuation in a fresh process, and uncertain load replay without canonical duplication.
  • Official Antigravity ACP 1.1.1 on macOS arm64 resumed the same external Session across processes and recalled a synthetic token. A loaded Session also recalled a synthetic token previously read by a tool after the toy file was removed. The built production Plugin bundles reported restorable → ready and recalled prior context in a fresh process. Sanitized evidence and the replay decision are in docs/archive/antigravity-acp-pr3-acceptance.md.

An earlier serial test run hit one intermittent Runtime Host Goal handoff assertion unrelated to ACP. The complete Host workspace passed independently, that test passed in isolation, and the final serial run passed.

Review focus

The official Agent replay did not expose stable canonical event identities for every observed turn. This PR deliberately reports a history gap for an uncertain prompt instead of guessing which replay notifications to append. The Plugin-private record binds the saved Session to the exact executable and helper bytes, workspace, and adapter identity. Process/Host restart is covered after a successful Plugin storage write; power-loss durability without fsync is not established.

Remaining draft verification

  • Exercise a full signed-in Desktop UI restart and capture the restored task surface. The real Agent protocol probe, built Plugin smoke, and Desktop picker/controller tests already pass separately.
  • Review CI results.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the ACP restoration path, tests, Desktop state, and acceptance documentation. Affected commits include Generated-by: OpenAI Codex trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Persist external Session identity and prompt checkpoints in Plugin storage. Resume committed turns through ACP, hold uncertain load replay outside canonical history, and expose explicit restore and history-gap states in Desktop.

Refs apache#5103

Generated-by: OpenAI Codex
@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 24, 2026
Acknowledge consumed terminal results and commit ACP checkpoints only after the agent has definitively settled. Preserve pending checkpoints for timeouts, crashes, and detached consumers; cover restart behavior with real stdio regression tests.

Generated-by: OpenAI Codex
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review September 26, 2026 06:25

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[kabi-grok-reviewer]

I reviewed 7eb6741ee884691b681aa655712b2912309b3dfc.

Design. #5103 is grounded: Maka as ACP client must continue the same external Antigravity Session after the ACP process or Host restarts. Persisting the Agent Session ID plus a versioned prompt checkpoint in Plugin-private storage, then calling negotiated session/resume, is that slice. Tasks without a saved ID stay history_only.

Storage. AcpContinuityRecord.version is the literal 2. decodeContinuity maps version === 1 to 'legacy' and anything else invalid to 'invalid'; both inspect as history_only. That fails closed on unknown future versions and is evolvable (a v3 can be added later). binding is sha256: of adapter + executable/helper bytes and is not sent to the Host.

Epoch. Code is RUNTIME_HOST_COMPATIBILITY_EPOCH = 190 with comment "Executor readiness exposes explicit restore, restore-failed and history-gap states." Live main is 189. The bump is required: ExecutorReadiness adds restorable | restoring | restore_failed | history_gap. The PR description's "183 → 184" is stale text, not the code.

Desktop UI. Catalog and picker copy include all four states. Actions: Restore on restorable/restore_failed; New Task on history_only/history_gap; restoring has no extra button. That matches the states. I did not click them in Electron.

docs/archive. antigravity-acp-pr3-acceptance.md is a dated probe log (including failed 403 gates), same shape as the PR 2 archive file. Evidence belongs in archive, not as the living contract.

P0–P2: none.

Checked this round: issue #5103 title/problem; protocol index.ts 107–108 vs origin/main 189; AcpContinuityRecord 113–127 and decodeContinuity 1027–1066; inspectConversation 236–275; executor-catalog.ts 35–41, 90–99; picker copy 68–95 and actions 313–318; archive doc 1–80; git merge-tree --write-tree origin/main 7eb6741ee exit 0, tree 700767fda.

Not checked: commit-after-terminal crash window, whether replay can leak into canonical transcript, restore retry, tests, Desktop restart, Antigravity process.

简体中文

我审查了 7eb6741ee884691b681aa655712b2912309b3dfc。#5103 站得住。存储 version: 2,未知版本失败为只读历史。epoch 代码是 190(PR 正文 183→184 是过时文字)。四个 Desktop 状态和按钮对得上。archive 文档当验收记录可以。没有 P0–P2。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[kabi-sol] Restore state-machine review of 7eb6741ee884691b681aa655712b2912309b3dfc: one P2; I do not recommend merging this revision.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

The inline finding concerns late external-session output escaping restoration quarantine while the history-gap checkpoint is being persisted. A controlled real-stdio Agent, the production SDK, AcpExecutor, PluginExecutorService, and PluginExecutorBackend reproduce it: the backend emits old text with the new, unsent Turn ID before returning the history-gap error. An isolated build changing only the notification guard to include session.historyGap eliminates that text event under the same schedule.

On this head, fresh dependencies and workspace dependencies/ACP plugin builds completed, and 83 existing targeted tests passed across the ACP executor/process and Runtime plugin backend/service suites. They exercise same-ID restoration after acknowledgement, uncertain restore replay, failed restore retry, corrupt/legacy record rejection, and terminal-consumption acknowledgement for completed/cancelled/failed results. These passing cases do not cover the reproduced late-notification window.

Evidence boundaries: the added stdio probe uses a controlled Agent and a delayed in-memory implementation of the Plugin state-store port. It checks the production backend's SessionEvent stream, not a full Host database/UI write. Existing real-process tests and backend acknowledgement tests were rerun; I did not kill the actual Host between its database commit and Plugin acknowledgement, run the official signed-in Antigravity Agent, a signed-in Desktop, Windows, or the full repository suite. CI test was still in progress at the checked snapshot. No approval, merge, or tracked production edits.

session.configOptions = restored.configOptions ?? [];
const restoredModel = currentAcpModel(session.configOptions);
if (restoredModel) await this.#persistConfirmedModel(session, restoredModel);
session.restoring = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep history-gap sessions quarantined until the connection is closed

For a saved prompt_pending record, session/load can return while the restored external session still has late output to deliver. This line clears restoring before the awaited history-gap write at line 602. execute() has already installed the new request's session.active (lines 365–367). During that storage wait, #acceptUpdate no longer takes its restoration return (lines 818–824), and for an agent_message_chunk with text and an active context it calls emitText(active.context, 'output_delta', update.content.text) at line 835; it does not check historyGap. The backend maps that output to text_delta using the new Turn ID (plugin-executor-backend.ts:392–400).

I reproduced this with a real stdio child Agent plus production Plugin/Service/Backend: after an unacknowledged first prompt, hold the history_gap store write and deliver one old-session text notification after the load response. The backend yields text_delta(turnId="unsent-new-turn", text="LATE OLD EXTERNAL OUTPUT"), then error and complete; the Agent history confirms only the original prompt was sent. The history-gap error therefore does not prevent old text from entering the new Turn's canonical event stream. The callback-level probe also leaks a tool start.

Keep updates quarantined throughout the gap/failure teardown, e.g. gate on historyGap as well as restoring, and add a late-notification regression. Adding just that guard in an isolated compiled copy makes the same stdio probe emit only error, complete.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated follow-up (OpenAI Codex): Confirmed and fixed in e0eccd0. Late Session updates are now dropped while the restored Session has a history gap or is being torn down; late permission requests are cancelled as well. The new regression blocks the history_gap checkpoint write, injects old text, tool activity, and a permission request, and verifies that none reaches the new Turn. The ACP Plugin suite passes (58/58).

@zhiiw zhiiw left a comment

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.

Independent review (blind — no existing comments read). Conclusions bind to 7eb6741ee884691b681aa655712b2912309b3dfc (test was still running at review time — no CI verdict claimed). The PR body's epoch note says "183 to 184"; the diff moves 189 → 190 with the right ledger comment — the body text is stale, the hunk is correct.

Verified locally (real Windows 11, Node 24.18.1 — the version CI pins), against the dispatch's three asks:

  • Plugin suites with real stdio + force-killed processes: acp-executor-plugin + acp-process — 49/52 pass, 2 skipped; all 3 failures are the test setup calling symlink(2) and getting EPERM (this machine has no developer mode) — a pre-existing environment class that fails identically on the base, not this PR. The real-stdio cases that matter here — killed process after durable ack, fresh process continuing the same Session, uncertain load replay — all pass.
  • Checkpoint write on Windows: the record writes go through pluginStateStore → PluginStorageService.set → HostPluginDataRuntime's atomicWrite (plugin-data-runtime.ts:297) — tmp file with wx, then rename. Atomic on Windows (same-dir rename replaces; the destination is never held open). No fsync on the file or the directory, so a power loss between rename and flush can drop the newest checkpoint — the failure direction is safe (a lost checkpoint restores as history-only / shows the explicit gap; nothing ever claims more than what was durably written). Not a finding; noting the bound so it is on record.
  • Ablation of the "committed only after the canonical terminal event is consumed" gate: the gate has two halves. Plugin-side (acknowledgeExecution's awaitingAck/phase/pendingTurnId check in acp-executor-plugin/src/index.ts:674) is a second belt — removing it alone changes nothing the tests observe. The load-bearing half is the caller gate in plugin-executor-backend.ts (returnedResult only exists once the queue's terminal event was accepted). Making acknowledgeExecution a no-op (scratch edit, reverted) → 10 tests fail, including the real-stdio restores the same Session in a new process after durable acknowledgement and the cancelled/end_turn/max_tokens/refusal checkpoint cases. After restore: green again. The commit path is pinned by real tests.

Read and agree with: the phase machine (reserved → established → prompt_pending → committed / history_gap) fails closed at every step — a failed checkpoint write flips to history_gap and loses the session rather than claiming continuity; restore never falls back to session/new; decodeContinuity rejects malformed records field-by-field (so a torn or old record degrades to history-only); the binding digest ties the saved Session to exact executable+helper bytes. The retry rule (only when no session/new was attempted and continuity was never reserved) prevents a lost response from double-creating an Agent Session.

Not verified: the official Antigravity ACP 1.1.1 resume (author's macOS run, cited in docs/archive/antigravity-acp-pr3-acceptance.md — not rechecked); the Desktop UI restart surface (the PR lists it as remaining draft verification); the full serial workspace sweep.

No P0–P3 findings.


Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.

@hqhq1025 hqhq1025 left a comment

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.

Finding

[P2] I independently corroborate the existing current-head inline finding about late restored-session output escaping the history-gap quarantine. execute() installs the new Turn's active context before initialization (packages/acp-executor-plugin/src/index.ts:357-368). On restoring a saved prompt_pending record, initialization clears session.restoring before its awaited history_gap write (packages/acp-executor-plugin/src/index.ts:583-603). During that await, #acceptUpdate gates only on session.restoring, then emits an Agent text chunk or tool event through the new context (packages/acp-executor-plugin/src/index.ts:818-843). The backend assigns that output the new Turn ID (packages/runtime/src/plugin-executor-backend.ts:392-400), even though the new prompt has not been sent. This can contaminate the canonical transcript before the history-gap error. The existing inline comment includes a real-stdio reproduction, so I am not duplicating it inline. Keep replay notifications quarantined until the gap path fully closes and add a regression for a notification arriving during the checkpoint write.

Scope and readiness

The 18-file PR adds a versioned Plugin-private ACP continuity record, executable binding, same-session resume/load and terminal-consumption acknowledgement, plus explicit Desktop readiness and restore controls. I inspected the Plugin restore/checkpoint state machine, Runtime backend acknowledgement boundary, Desktop restore/configuration path, catalog/epoch contract, and adjacent tests. The configuration patch intentionally invokes the provider even when the selected model is unchanged (packages/runtime-host/src/server/session-catalog-coordinator.ts:823-853), so the Restore action can reach initialization. I found no other substantiated P0–P3 issue in that scope.

Current-head test and label checks pass; the diff check and synthetic merge onto fetched main are clean. The P2 remains a blocker despite those checks. I did not run local tests (Node 18/no dependencies), the signed-in Antigravity Agent, a real Host restart, or Desktop UI interaction. No database schema/migration files changed. This is not a merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Keep history-gap and lost sessions from projecting late Agent output or permissions into a newly requested turn. Cover notifications arriving while the gap checkpoint write is pending.

Generated-by: OpenAI Codex
@Sun-GLiang

Copy link
Copy Markdown
Member Author

Automated follow-up (OpenAI Codex): A correction to the Windows checkpoint note in @zhiiw's review: the absence of fsync does not guarantee that a lost checkpoint becomes history_only or history_gap. For example, #beginPrompt writes prompt_pending before sending the ACP prompt. A power loss could discard that unflushed write and leave the previous committed record, even if the Agent received the prompt. Restart would then take the session/resume path and could treat the history as aligned. The current process-kill and Host-restart tests do not establish power-loss durability, so I have made that boundary explicit in the PR description. The epoch note is also corrected to 189 → 190.

@hqhq1025 hqhq1025 left a comment

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.

Reviewed current head e0eccd0c7d650b6481c22cfea4653078c7b51244, focusing on the new ACP restore-quarantine increment and its call path. I found no substantiated remaining P0–P3 issue in that inspected path. The previous P2 is addressed on this head: the prompt_pending restore sets historyGap before awaiting its durable checkpoint (packages/acp-executor-plugin/src/index.ts:586-607), and late updates and permission requests now stop on that flag or a lost session (packages/acp-executor-plugin/src/index.ts:804-845). The added regression blocks the checkpoint write and injects late text, tool activity, and permission, asserting no new-Turn events escape (packages/acp-executor-plugin/src/__tests__/acp-executor-plugin.test.ts:182-246). I read the existing current-head follow-up and did not repeat its inline comment.

The full PR spans ACP execution/restoration plus related runtime, Desktop, UI, tests, and docs; this pass concentrated on the new two-file increment and its adjacent lifecycle. No schema/migration files change. Current-head test succeeds and PR diff-check is clean, but the branch conflicts with freshly fetched main in packages/runtime-host/src/protocol/index.ts; GitHub reports it unmergeable. I did not run local tests, a signed-in external Agent, Host restart, or Desktop UI validation. Resolve the conflict and recheck the resulting head before human merge consideration. This is not a merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants