Repository navigation
pids.json 加锁读改写；派发与终态响应先落盘再返回 - #34
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 08007008-d6e9-484c-a96a-9af31e1fd4eb) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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: 8c0d4b389f
ℹ️ 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".
| if task.status not in TERMINAL_STATUSES: | ||
| return None | ||
| if task.task_id not in self.tasks: | ||
| return True | ||
| return await self.flush_state() |
There was a problem hiding this comment.
A terminal status does not mean the terminal snapshot has been queued for writing. For example, _stall_watch sets failed before awaiting worker cancellation, and _run_task sets failure states before awaiting final file-change collection; the only terminal save() occurs later in _run_task's finally. During that window, check_task, get_result, or cancel_task reaches this helper, flushes only the previously queued running snapshot, and returns persisted: true, so a crash can still lose the terminal state despite the new guarantee. Gate this verdict on _done/finalization or associate it with the generation created by the terminal save.
Useful? React with 👍 / 👎.
| delay = PIDS_WRITE_RETRY_BASE_SEC | ||
| for attempt in range(PIDS_WRITE_ATTEMPTS): | ||
| try: | ||
| with file_lock(lock_path, timeout_sec=PIDS_LOCK_TIMEOUT_SEC): |
There was a problem hiding this comment.
When another Bridge instance holds pids.json.lock, file_lock polls with synchronous time.sleep for up to five seconds, and the PermissionError retry path sleeps synchronously as well. record_pid and drop_pid are called directly from async adapter startup/shutdown paths, so lock contention or transient Windows sharing failures freeze the server's entire event loop, delaying unrelated MCP calls and watchdogs. Run this locked read-modify-write in a worker thread or otherwise make the wait asynchronous.
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 5a4040da-70ce-47a6-ae55-1b1118d8b83a) |
| try: | ||
| persisted = await self.flush_state() | ||
| finally: | ||
| # Even when the caller cancels mid-flush, the registered task | ||
| # must still start — a request_id retry would otherwise reuse | ||
| # a row that never runs. | ||
| self._bg[task.task_id] = asyncio.create_task(self._run_task(task.task_id), name=f"task-{task.task_id}") |
There was a problem hiding this comment.
🔴 Cancelled dispatch can run without durable binding
When flush_state is cancelled before writing, dispatch_task starts the worker without a durable request binding. A restart and request-id retry can dispatch the same prompt twice.
Learn more
The initial save contains both the queued task and its request-id binding. Cancellation ends the caller's wait on flush_state without guaranteeing the background write completed. The finally block starts _run_task before that initial generation is durable. If Bridge exits while the worker processes the prompt, start cannot recover the binding and a retry creates another task.
Example: A caller sends request ID R, then its MCP call times out during a slow initial write. The worker receives the prompt, but Bridge exits before state.json contains R. A retry of R after restart starts the prompt again.
Recommended fix: Keep the initial state write alive on cancellation and gate _run_task on its completion. Decide explicitly how to handle persistent write failure before execution, rather than treating cancellation as proof the binding was saved.
Was this helpful? React with 👍 or 👎 to provide feedback.
| done = self._done.get(task.task_id) | ||
| if done is None or not done.is_set(): | ||
| # _run_task marks the status terminal in-memory, then collects | ||
| # final files_changed and flushes the transcript before its | ||
| # closing save. Done set means that save has been issued (it | ||
| # runs synchronously right after set()); before that, issue the | ||
| # save here so a read cannot answer persisted=True while disk | ||
| # still says running. | ||
| self.save() | ||
| return await self.flush_state() |
There was a problem hiding this comment.
🟡 Terminal answers can preserve unfinished file lists
When check_task sees a failed task during final file collection, _persisted saves its pending file list. It can return persisted=True before the final list reaches disk, leaving a restart with unavailable changes.
Learn more
On the failure path, _run_task marks a task terminal before its final workspace merge, finished_at update, transcript flush, and final save. _persisted now saves that intermediate row and waits only for its generation. Once finalization resumes, it issues another save, but the earlier flush can already report success; a crash before the second save leaves incomplete terminal metadata on disk. start converts a pending file list on a recovered terminal task into unavailable.
Example: A failed turn changed src/a.py. check_task saves status=failed and files_changed_state=pending while the final merge runs; it reports persisted=True. A crash before the closing save causes the recovered task to report files_changed_state=unavailable, losing src/a.py from the task metadata.
Recommended fix: Treat finalization as incomplete until _done is set and the closing save generation has been flushed. Wait for that generation before declaring the final terminal response persisted, rather than saving a partial terminal snapshot in _persisted.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
自动审查结论处理：
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
未发现高置信问题。
上一轮报的 _persisted 终态误报（磁盘仍是 running 却返回 persisted=true），以及 dispatch_task 在 flush_state 期间被取消后任务滞留，已在 7dbd964 修复：_done 未置位时先 save() 再 flush；_run_task 放到 finally 里启动。pids.json 加锁读改写与 reap_orphans 快照比对路径未见新的高置信正确性缺陷。
Sent by Cursor Automation: GitHub PR 自动审核
Summary
record_pid/drop_pid/reap_orphans在pids.json.lock文件锁内完成读改写，PermissionError按 state.json 相同参数退避重试。reap_orphans锁内取快照、锁外杀进程，再只删除本次处理过且未被改动的条目。dispatch_task在启动 worker 前把任务行和request_id绑定写盘，避免崩溃后重试导致重复派发。check_task/get_result/cancel_task返回终态前等待写盘，和wait_task一样带persisted（新增字段，非终态不带）。check_task/get_result在 Registry 中改为 async。persisted的说明。Closes #26
Closes #27
Test plan
pids.json保留全部 90 条check_task看到终态时，磁盘上已是终态dispatch_task返回时，任务行与request_id绑定已在磁盘persisted=false，结果仍可读Note
Medium Risk
Changes core dispatch persistence and concurrent
pids.jsonbehavior; mistakes could cause duplicate dispatches or orphan-worker handling issues, though extensive tests cover the main races.Overview
Hardens multi-instance
pids.jsonupdates and makes task API responses honest about disk persistence.record_pid/drop_pid/reap_orphansnow do locked read-modify-write onpids.json(with retries on transientPermissionError). Unreadable tables are no longer treated as empty, and orphan reap only drops rows that still match the snapshot so concurrent sibling registrations are not wiped.Task coordination:
dispatch_taskflushesstate.json(task row +request_idbinding) before the worker starts and returnspersisted; the worker still starts if the MCP call is cancelled mid-flush. Terminal answers fromwait_task,check_task,get_result, andcancel_taskalso includepersistedafter ensuring the terminal row is flushed (including the gap before_run_task’s final save).check_taskandget_resultare async through the MCP layer.ORCHESTRATION (EN/zh) and the agent-bridge skill document the new
persistedfield.Reviewed by Cursor Bugbot for commit 7dbd964. Bugbot is set up for automated code reviews on this repo. Configure here.