fix: fence uncertain approved write replays - #61
Conversation
c8696e7 to
7a87ddb
Compare
tuanle96
left a comment
There was a problem hiding this comment.
Blocking concurrency issue: the approval claim is not atomic.
execution_status is read near the start of _execute_approved_write_gated, but it is only changed to external_attempt_started immediately before odoo.execute_method. With concurrent MCP requests, two callers can both observe validated, pass the remaining gates, and each issue the outbound mutation.
I reproduced this deterministically on the current head 7a87ddb4d521fef52b6896a33a8d2f2c89fcae15 by synchronizing two calls after the status check and before the transition:
{'outbound_mutations': 2,
'codes': ['external_result_uncertain', 'external_result_uncertain']}
The new sequential replay test passes, but it does not cover this race.
Please claim the approval atomically before the outbound call and add a concurrent regression test proving that two simultaneous executions of the same approval produce at most one outbound mutation. Keep the current fail-closed external_result_uncertain behavior for the claimed execution when its response is lost.
|
Fixed in The approval is now claimed atomically under a per-lifespan lock immediately before the outbound Odoo call; the call itself remains outside the lock. Revalidation cannot reset an The deterministic two-thread regression synchronizes both callers after their initial validation read. Exactly one caller reaches the synthetic destination, the other receives Validation: Ruff passed, mypy passed (34 source files), 214 focused tests passed, and the portable full suite passed (916 passed, 1 known Windows-only symlink-race test deselected). |
tuanle96
left a comment
There was a problem hiding this comment.
Re-reviewed head 4e2259768d0b958d6105454975329ba32f37138e after the concurrency blocker was addressed.
The approval is now claimed under write_approvals_lock, the claim path rechecks payload/status/expiry atomically, and revalidation cannot reset an in-progress or uncertain record. The new deterministic concurrent regression proves two simultaneous callers produce at most one outbound mutation and preserves the fail-closed uncertain-result behavior.
Verification:
- local full suite: 917 passed
- Ruff: passed
- mypy: passed across 34 source files
- import-linter: 2/2 contracts kept
- GitHub Actions run 30423671673: Python 3.10/3.11/3.12, build, and Odoo 19 MCP Inspector all passed
Approved.
Summary
Fence an approved write when its outbound mutation starts. If Odoo may have committed the write but the response is lost, mark the approval
external_result_uncertain, reportretry_safe: false, and block replay.This tightens the existing write gate. It does not add automatic retries, destination reconciliation, or an exactly-once claim.
Coverage
The regression test uses a synthetic destination, so no live Odoo instance or transport is required. XML-RPC, JSON-2, stdio, Streamable HTTP, and SSE behavior is unchanged.
Verification
One unrelated timing test failed once, passed on rerun, and passed in the next full run.
Checklist
CHANGELOG.mdunder## UnreleasedSafety boundary
The existing preview, validation, session binding, expiry, confirmation, and write-enable checks remain. An exception after possible destination contact permanently fences that approval within the server session. The test commits one synthetic create, drops its response, retries the approval, and confirms the mutation count stays at one.
The state names follow the vendor-neutral Reliability Sidecar Contract v1, but this implementation is fully local and has no hosted dependency.