Fix assistant job cancel flag compile failure#122
Conversation
mAgentRunGeneration was a non-volatile int written on the UI thread (runAgent, cancelAgentRun, live-voice starts) and read from the agent background thread inside isCancelled(). Without synchronization the agent thread has no visibility guarantee for writes from the UI thread, so a cancellation could go unobserved. Converted to AtomicInteger and updated every read/write site to use incrementAndGet()/get(). OpenPhoneAgentJobScheduler.isCancelled() hardcoded false, so a background agent_turn job could never be stopped once its thread started. Added a per-job AtomicBoolean cancel flag (keyed by job id in a ConcurrentHashMap) and a new cancelJob(String) entry point that sets it; the flag is cleared in the job's finally block. I also reviewed the pending-confirmation fields (mPendingActionId, mPendingToolName, mPendingToolArguments, mPendingOneShotMessage) called out in the issue. Every write site I found is either inside a runOnUiThread block or a method only invoked from the UI thread, and the one place a background Thread reads them (confirmPending) does so via local finals captured before Thread.start(), which is safe under Java's happens-before guarantee. I did not find an actual race on these fields, so I left them unchanged rather than add synchronization for a scenario I could not reproduce or confirm — flagging this in the PR in case I've missed a call path. Fixes #70
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 926f37689b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public boolean isCancelled() { | ||
| return false; | ||
| return cancelFlag.get() || Thread.currentThread().isInterrupted(); |
There was a problem hiding this comment.
This cancellation check never becomes true from the actual background_job_stop tool: FrameworkToolExecutor.backgroundJobStop only updates AgentJobStore.stop(id) and schedules the next alarm, and a repo-wide search shows no caller for the new OpenPhoneAgentJobScheduler.cancelJob(...). When a built-in agent_turn job is already running, stopping it therefore leaves the model loop running until it finishes, after which store.markCompleted(...) can overwrite the stopped status; please signal this flag from the stop path and avoid completing canceled jobs as successful runs.
Useful? React with 👍 / 👎.
2 participants
Summary
Longkeys matchingAgentJobRecord.id.cancelJob(String)argument before looking up the running job flag.This branch is based on PR #103 (
28d1b48d) because the compile failure is introduced there; the final commit is the minimal fix (926f3768).Evidence
./scripts/check.shfailed on PR fix(assistant): thread-safety in agent run loop and job cancellation #103 atOpenPhoneAgentJobScheduler.java:126:incompatible types: long cannot be converted to String.AgentJobRecord.idis along, while the newsCancelFlagsmap was declared asConcurrentHashMap<String, AtomicBoolean>.Validation
./scripts/check.shgit diff --checkNotes