Fix data races on OutstandingAction result state - #1064
Conversation
The handler goroutine wrote Rv/Annos with a plain assignment while the invoke select arms, GetActionStatus, and CleanupOldActions read them without the mutex, and setError mutated a published structpb.Struct in place. Route writes through a locked setResult, reads through a locked result() snapshot, and rebuild the response struct in setError instead of mutating it.
7a5031d to
e90ef4b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: Fix data races on OutstandingAction result stateBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe full PR diff was re-scanned for security and correctness; the new commit is scoped to Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
SetError now updates the response and status in one critical section so no snapshot observes the error with a non-terminal status. Result() gives holders of an OutstandingAction outside the package a race-free read of the outcome. The cleanup race test pins its target action to a strictly earlier StartedAt, since the unstable sort could otherwise move it out of the visited prefix on tied timestamps, and documents make race-check as the gate that runs it under the race detector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
see also kans/lock-safety |
… the writer The handler goroutines published the result and the terminal status in two critical sections, leaving the same tearing window SetError just closed; setOutcome now publishes both under one lock. The cleanup race test waits for the writer's first status write before racing cleanup against it — on a loaded runner the goroutine could be scheduled only after close(stop), never write, and fail the RUNNING assertion — and its doc comment now describes the assertions and the out-of-band race-check gate accurately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lifecycle transitions are now enforced: terminal statuses reject every transition (a FAILED action may still take a later, richer error as a replacement account), RUNNING is only reachable from PENDING, and the old always-assign path that let a late handler completion flip a cancelled action to COMPLETE is gone. The rejected-transition logs fire during normal cancel/complete races, so they are debug level now. The cancellation select arm re-checks the done channel and prefers a completed result over a spurious cancellation return. Handler results are cloned at the publication seam: the handler owns what it returned and may keep mutating it, so publishing its pointers raced concurrent snapshot marshals. The cleanup race test writes status under the lock directly, since single-shot transitions mean no public API writes status repeatedly; new deterministic tests cover the transition table, cancel-then-late completion, error replacement, and publication isolation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A cancelled request marks the action FAILED, but the handler keeps running detached and its side effects can still complete; treating that FAILED as final meant permanently reporting failure for an action that succeeded. Cancellation now sets a provisional mark that the handler's own outcome — success or failure — replaces, while a real handler failure stays final. The cancellation return also pairs totally: if the handler completed by the time the cancel arm takes the lock, the call returns the completed result with no error instead of a COMPLETE snapshot alongside a cancellation error. Annotations are deep-copied at the publication seam, since the slice-header copy left elements shared with the handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lacement Cleanup treated every FAILED action as evictable, but a cancellation-FAILED record is provisional: its handler is still running and its late outcome is supposed to replace the status. Evicting it first orphaned that replacement and turned a late success into NotFound. Eviction now asks the action whether it is truly terminal, skipping provisional cancellations; the status read moves inside the action's lock along with the mark. The provisional replacement is also the only path that regresses a terminal status, and it was the only transition with no log line. It now emits a debug record with the action id and name, so a FAILED-then-COMPLETE observation can be traced. New coverage: cleanup retains a provisional record and its late success stays observable; a panic after cancellation clears the provisional mark and is final; cancelling an already-COMPLETE action is rejected outright; and a 200-iteration concurrent cancel/complete sampler pins the status/error pairing contract on every interleaving. The cleanup race canary was re-validated against the new read path: an unlocked evictable() plant fails under -race, the real code passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| for i := 0; i < len(actionList)-maxOldActions; i++ { | ||
| action := actionList[i] | ||
| if action.Status == v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE || action.Status == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED { | ||
| if actionList[i].evictable() { |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): evictable() correctly keeps a provisional cancellation alive so the handler's late outcome stays observable, but there's no age backstop — a handler that ignores handlerCtx and never returns leaves cancelled == true forever, so that record can never be evicted. The eviction window widens as the map grows, so this isn't unbounded growth, and such a handler already leaks its goroutine (a bigger problem than the record). Still, consider treating a provisional action older than the 1h handler timeout as evictable, since by then no legitimate late outcome can arrive.
|
(windows test failure here: Not caused by this branch. The failing test came in on main with the chaos-connector PR (305b2a5, #1052), and this branch's entire diff is two files in pkg/actions, which is nowhere on the sync path. What failed is the test's own premise gate, and it's a known failure class in this repo with two prior fixed instances. The mechanics. chaos_cancellation_test.go:78 is a premise check: after starting Sync in a goroutine, require.Eventually polls for ActiveOperations() > 0 — meaning the sync has reached the deterministically-blocked ListEntitlements call — with a 2-second window. Before that call can block, the subtest must do a full baseline sync plus read-back, then the chaos sync's own setup: open a fresh c1z (sqlite + zip on disk), sync-run bookkeeping, the resource-types pass, and the resources pass. All of that is disk I/O on a Windows CI runner, where it's several times slower than Linux. The condition never became true within 2 seconds, so the gate failed with "Condition never satisfied." The sync context's 3-second deadline (chaos_cancellation_test.go:70, armed before harness construction) compounds it — on a slow enough runner the deadline can expire before the block is ever established, so there is no state the poll could ever observe. ) going to re-run for good measure. |
passed on re-run |
2 participants
The handler goroutine wrote Rv/Annos with a plain assignment while the invoke select arms, GetActionStatus, and CleanupOldActions read them without the mutex, and setError mutated a published structpb.Struct in place. Route writes through a locked setResult, reads through a locked result() snapshot, and rebuild the response struct in setError instead of mutating it.