fix(parallel): report a detail-less panic as unavailable, never as another task's - #26
Merged
Merged
Conversation
…other task's The close-out of #19, Option 1. The cardinality fix itself landed with the substrate's per-instance keying (#25 here, lisachenko/php-shared-data-extension#23): each panic is its own SharedError instance graph whose address rides in the panicking task's own slot, so two concurrent panics can no longer share one error entry. What remained was pinning the acceptance criteria and making the detail-unavailable path tell the truth. - The fallback for a panic slot whose payload does not attach as a SharedError was a WorkerCrashedException — but the worker settled the slot, so it is demonstrably alive, and a waiter catching ParallelTaskException never saw the panic at all. It is now a ParallelTaskException that states the detail is unavailable, with no fabricated class and no borrowed trace — the same shape the no-arena supervisor path already uses for a detail-less PANIC record. - testEachPanicKeepsItsOwnSharedError now pins all three fields of the first acceptance criterion: the two concurrent panics differ by class, message and task frame, and each waiter asserts its exception carries its own class, its own message AND its own trace — with the other task's frame asserted absent. - New testAPanicWhoseDetailCannotBeAttachedSaysSoExplicitly pins the third criterion by settling a panic slot by hand with the address of a shared object that is not a SharedError: the waiter still sees a task panic, told explicitly that the detail is unavailable. The second criterion — no panic path serializes anything — is the existing source-scanning test, untouched and still green. Closes #19 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U5QRWPfmkZME7hDjU6i5EA
…rsist() The docblock still described the substrate's old registry contract — class-keyed storage where a second persist of one class is an upsert — while the method body beneath it has ridden persistInstance() since the per-instance keying landed. Say what the code does: one entry per graph, named by its own root address, any number of one class live at once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U5QRWPfmkZME7hDjU6i5EA
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.
Closes #19 — Option 1, as directed in the issue thread: straight to one error object per panic, no interim panic-id detection.
Where Option 1 already stood
The cardinality fix itself landed as a by-product of #15: the substrate keys persisted graphs per instance (lisachenko/php-shared-data-extension#23), and
SharedError::capture()ridespersistInstance()— each panic is its own three-string instance graph whose address travels in the panicking task's own slot. Lifetime and identity are the slot's: the detail is reachable exactly as long as the slot's payload points at it, which is what Option 1 asked for. (The #16 interplay is ready when slots become reclaimable:dropInstance($address)is the substrate hook for reclaiming a slot's error object with it.)This PR closes out the issue by pinning all three acceptance criteria and fixing the one place the runtime could still misreport.
What changed
SharedErrorused to surface as aWorkerCrashedException— but the worker settled the slot, so it is demonstrably alive, and a waiter catchingParallelTaskExceptionnever saw the panic at all. It is now aParallelTaskExceptionthat states the detail is unavailable, with no fabricated class and no borrowed trace — the same shapeWorkerSupervisor::panicFor()already uses for a detail-lessPANICrecord on the no-arena path.testEachPanicKeepsItsOwnSharedErrornow pins the full first criterion. The two concurrent panics differ by exception class, message and task frame (PanicWithMessageTask→RuntimeException, newPanicWithDomainErrorTask→DomainException), and each waiter asserts its exception carries its own class, its own message and its own trace — with the other task's frame asserted absent from it.testAPanicWhoseDetailCannotBeAttachedSaysSoExplicitlypins the third criterion: a panic slot settled by hand with the address of a shared object that is not aSharedErrorstill surfaces as a task panic, told explicitly that the detail is unavailable — never another object's fields presented as this task's failure.SharedArena::persist()'s docblock (left behind by feat(parallel): let any number of same-class tasks be in flight at once #25) now describes the per-instance keying; the AGENTS.md panic bullet records the fallback contract.Acceptance criteria from #19
ParallelTaskExceptions, each carrying its own class, message and trace —testEachPanicKeepsItsOwnSharedError.phpt, strengthened as above.testAPanicWhoseDetailCannotBeAttachedSaysSoExplicitly.phpt.Verification
PHP 8.4.19,
-d ffi.enable=1 -d opcache.jit=off, local vendor tree (8.4 z-engine line; the 8.5 matrix leg runs in CI, which resolves its own line):OK (98 tests, 98 assertions)— 97 existing + 1 new, no leftover children.composer phpstan— level max, no errors.composer cs:check— clean.The new fallback test discriminates by construction: under the previous behaviour the raised
WorkerCrashedExceptionescapes itscatch (ParallelTaskException)block and fails the run.🤖 Generated with Claude Code
https://claude.ai/code/session_01U5QRWPfmkZME7hDjU6i5EA
Generated by Claude Code