Conversation
`current::clock()` unconditionally calls `ExecutionState::current()`, which
unwraps `current_task.id()`. During `ExecutionState::cleanup` the current task
is `Stopped` or `Finished`, so that unwrap panics.
This is reachable from ordinary user code. Every `BatchSemaphore` operation
calls `clock()`, so any `Drop` handler that touches a modelled `Mutex`,
`RwLock` or semaphore reaches it. Cleanup force-unwinds parked tasks, so a task
that is still parked when an execution ends runs its destructors from cleanup.
If such a destructor drops the last handle to a structure whose own `Drop`
takes a modelled lock, the unwrap fires inside a destructor and Rust escalates
it to a non-unwinding abort: the process dies and the failure report, the
shrunk counterexample and the schedule file are all lost.
Observed as an `exit 250` abort in a large soak run, from:
ExecutionState::cleanup
Continuation::drop -> force_unwind of a parked task
<user task drops its last Arc>
<user Drop> -> Mutex::try_lock
BatchSemaphore::try_acquire -> acquire_permits -> current::clock()
ExecutionState::current() -> current_task.id().unwrap() <- None
Use `try_current()` and return an empty clock when there is no current task.
An operation that belongs to no task has no causality to record, so an empty
clock is the correct value rather than a fallback.
This complements awslabs#346. That change stops the portfolio runner from leaving a
stopped execution visible to drop handlers; this one makes the clock lookup
itself safe, which also covers single-runner executions reaching cleanup after
an ordinary failure.
Note that the `vector-clocks` feature does not avoid this. It stubs the
`VectorClock` type, but `clock()`, `get_clock` and the `BatchSemaphore` call
sites are not gated, so a clocks-off build still performs the panicking lookup
and then discards a zero-sized result.
`ExecutionState::with` panics on both `NotSet` and `AlreadyBorrowed`, so the previous version still aborted when a destructor re-entered `ExecutionState` while it was already borrowed -- the panic hook does exactly that while trying to name the failing task. Switch to `try_with` so all three cases degrade to an empty clock: no `ExecutionState`, state already borrowed, and no current task.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
current::clock()calledExecutionState::withand thenExecutionState::current(), which unwrapscurrent_task.id():There are three ways that aborts the process rather than reporting a failure:
ExecutionState::cleanupthe current task isStoppedorFinished, soid()isNoneand the unwrap panics.AlreadyBorrowed.withpanics ifExecutionStateis already borrowed, which happens when a wake re-enters it — including from the panic hook while it tries to name the failing task.NotSet.withpanics outside a Shuttle execution.All three are reachable from ordinary user code, because every
BatchSemaphoreoperation callsclock()— so anyDrophandler that touches a modelledMutex,RwLockor semaphore reaches it. Cleanup force-unwinds tasks that were still parked when the execution ended, which runs their destructors outside any task context.Because the panic originates inside a destructor, Rust escalates it to a non-unwinding abort. The process dies and the failure report, the shrunk counterexample and the persisted schedule are all lost — so the user sees an abort instead of the bug they were actually hunting.
We hit this once in a 1000-job soak run, as
exit 250:Fix
Use
ExecutionState::try_withtogether withtry_current(), so all three cases degrade to an empty clock instead of panicking:An operation that belongs to no task has no causality to record, so an empty clock is the correct value rather than a fallback.
try_withandtry_currentboth already exist and are used defensively elsewhere.Relationship to #346
This complements #346 rather than duplicating it. #346 stops the portfolio runner from leaving a stopped execution visible to drop handlers, by extending
force_resetto leak state. This change makes the clock lookup itself safe, which also covers single-runner executions that reachcleanupafter an ordinary failure — the case we hit, which #346 does not address.clock()still unwraps onmainas offa6f0be.Note on the
vector-clocksfeatureDisabling vector clocks does not avoid this. The feature stubs the
VectorClocktype, butclock(),ExecutionState::get_clockand theBatchSemaphorecall sites are not gated. A clocks-off build therefore still performs the panicking task lookup and then throws away a zero-sized result.That also suggests a separate, optional improvement: gating the
BatchSemaphorecall sites on the feature would remove a per-acquire task lookup from every clocks-off build. Happy to do that in a follow-up if it is wanted — it is a performance change rather than a correctness one, so it is deliberately not in this PR.Testing
cargo check -p shuttle-enginepasses. I have not added a regression test: reproducing this needs an execution that ends with a task still parked whose unwind drops a modelled primitive, and I would rather have a maintainer confirm the intended shape than guess at one. Happy to add a test in the style of theportfolio_stop_force_unwinds_atomic_droptest from #346 if you point me at the preferred harness.