@@ -29,12 +29,15 @@ private[nbtest] final class FabricTestArtifactTracker(deleteArtifact: String =>
         deleteTrackedArtifact(artifactId)
         artifactIds.remove(artifactId)
       } catch {
-        case cleanupError: Throwable =>
+        case NonFatal(cleanupError) =>
           failure match {
             case Some(original) =>
               if (original ne cleanupError) original.addSuppressed(cleanupError)
             case None => throw cleanupError
           }
+        case cleanupError: Throwable =>
+          failure.filterNot(_ eq cleanupError).foreach(cleanupError.addSuppressed)
+          throw cleanupError
       }
     }
   }
 
@@ -6,8 +6,55 @@ package com.microsoft.azure.synapse.ml.nbtest
 import org.scalatest.funsuite.AnyFunSuite
 
 import scala.collection.mutable.ArrayBuffer
+import scala.util.control.ControlThrowable
 
 private[nbtest] trait FabricTestArtifactTrackerFailureTests extends AnyFunSuite {
+  test("Propagate fatal per-artifact cleanup errors after successful or failed work") {
+    val cleanupFailures = Seq[() => Throwable](
+      () => new InterruptedException("cleanup interrupted"),
+      () => new InternalError("cleanup VM failure"),
+      () => new ThreadDeath(),
+      () => new LinkageError("cleanup linkage failure"),
+      () => new ControlThrowable {})
+    cleanupFailures.foreach { newCleanupFailure =>
+      Seq[Option[Throwable]](None, Some(new IllegalStateException("job failed")),
+        Some(new InternalError("job VM failure"))).foreach { jobFailure =>
+        val cleanupFailure = newCleanupFailure()
+        var attempts = 0
+        val tracker = new FabricTestArtifactTracker(_ => {
+          attempts += 1
+          if (attempts == 1) throw cleanupFailure
+        })
+        val thrown = intercept[Throwable] {
+          tracker.withArtifact("job") { _ =>
+            jobFailure.foreach(throw _)
+            "completed"
+          }
+        }
+        assert(thrown eq cleanupFailure)
+        val suppressionProbe = newCleanupFailure()
+        suppressionProbe.addSuppressed(new RuntimeException("suppression probe"))
+        val expectedSuppressed = if (suppressionProbe.getSuppressed.isEmpty) Seq.empty else jobFailure.toSeq
+        assert(thrown.getSuppressed.toSeq == expectedSuppressed)
+        assert(attempts == 1)
+        tracker.cleanup()
+        assert(attempts == 2)
+        tracker.cleanup()
+        assert(attempts == 2)
+      }
+    }
+  }
+
+  test("Preserve a repeated fatal cleanup throwable without self-suppression") {
+    val failure = new InterruptedException("cleanup interrupted")
+    val tracker = new FabricTestArtifactTracker(_ => throw failure)
+    val thrown = intercept[InterruptedException] {
+      tracker.withArtifact("job") { _ => throw failure }
+    }
+    assert(thrown eq failure)
+    assert(thrown.getSuppressed.isEmpty)
+  }
+
   test("Attempt all deletions and preserve cleanup failures") {
     val attempted = ArrayBuffer.empty[String]
     val firstFailure = new RuntimeException("first failure")
 
@@ -86,6 +86,10 @@ cleanup or starting notebook work; successful notebook runs retain their bounded
 parallel execution and per-job artifact cleanup. Interrupted cleanup preserves
 the interrupt signal. Store creation and executor setup failures are also
 cached, so later tests do not repeat initialization or start another notebook batch.
+Per-job cleanup never suppresses an interrupt or fatal error behind a notebook
+failure. The cleanup throwable escapes with the earlier notebook failure attached
+where that throwable permits suppression. An unsuccessful per-job deletion stays
+tracked for final cleanup.
 
 Set `SYNAPSEML_FABRIC_CLEANUP_DRY_RUN=true` to preview eligible deletions without
 changing the workspace. Omit it, or set it to `false`, to perform cleanup.
 
@@ -0,0 +1,42 @@
+## Review summary
+
+- Round: 1, broad sweep only
+- Theme: Correctness, security, logic, and conformance to the requested cleanup behavior
+- Mode: sequential
+- Model: gpt-6-astra
+- Target: master follow-up for microsoft/SynapseML#2734, comment 4066689707
+- Base HEAD: `02272e0a5d6f986a09289149310f5475c2457b1b`
+- Scope: The uncommitted three-file delta and relevant tracker context
+- Artifact: `reviews\fabric-cleanup-relations-20260921\task-fatal-cleanup-attempt-1-review-1-gpt-6-astra.md`
+- Issues found: 0
+- Verdict: CLEAN
+
+## Reviewed working-tree snapshot
+
+| Repository-relative path | Git blob hash |
+| --- | --- |
+| `core\src\test\scala\com\microsoft\azure\synapse\ml\nbtest\FabricTestArtifactTracker.scala` | `8ddef631521c4730b20dbd3d9dd93efbc51ecfee` |
+| `core\src\test\scala\com\microsoft\azure\synapse\ml\nbtest\FabricTestArtifactTrackerFailureTests.scala` | `717c93f7b504ab7b29eb6bc6d3a10497b0baf895` |
+| `docs\Reference\Developer Setup.md` | `9701a07632698d1b0ba7211da0ebc1165bb9436d` |
+
+## Evidence checklist
+
+- [x] Inspected the exact delta against the stated HEAD. The implementation changes only the per-artifact cleanup catch from `Throwable` to `NonFatal`; the body catch and rethrow are unchanged.
+- [x] Traced successful work, ordinary body failure, and fatal body failure. An excluded cleanup throwable escapes the `finally` block unchanged rather than becoming suppressed behind the body error.
+- [x] Checked ordinary cleanup behavior remains intact: preserve a prior body error with distinct cleanup errors suppressed, or throw the cleanup error when the body succeeded. The self-suppression guard is unchanged.
+- [x] Checked artifact retention. Cleanup failure exits before deque removal, permitting final cleanup to retry; successful cleanup removes the entry and prevents another deletion.
+- [x] Reviewed all 15 regression combinations: `InterruptedException`, `InternalError` as a `VirtualMachineError`, `ThreadDeath`, `LinkageError`, and `ControlThrowable`, each with successful, ordinarily failed, and fatally failed work.
+- [x] The regression uses fresh cleanup throwable instances, asserts exact identity and attempt counts, retries the retained artifact, then checks that a second final cleanup performs no duplicate deletion.
+- [x] Confirmed the private trait is mixed into the CI-selected `FabricTestArtifactTrackerSuite`. The new case appears under that concrete suite in the green log.
+- [x] Checked documentation matches the exception precedence and retained-artifact behavior. No public signature, network destination, credential handling, or serialization changes are introduced.
+- [x] Inspected `master-fatal-cleanup-red.log`: the new test fails on old code because the body `IllegalStateException` escapes instead of the same cleanup `InterruptedException`.
+- [x] Inspected completed `master-fatal-cleanup-green.log`: JDK 11 core compile and Test/compile succeeded; main and test scalastyle report zero errors and warnings; 47 tests passed across two completed suites, with zero failures, canceled, ignored, pending, or aborted cases.
+- [x] The focused working-tree diff passes whitespace checking. No scoped changes were staged during this review.
+
+## Conclusion and boundary
+
+No concrete defect found in this bounded delta. The fix restores propagation of interruption, fatal JVM errors, and control throwables without changing ordinary failure handling or artifact retry bookkeeping.
+
+Validation claims above come from inspected parent-produced logs, not reviewer-run builds. Only this report was written; no source edits, agents, staging, commits, pushes, or remote calls were performed. No new port validation is claimed.
+
+The strict Gemini-family gate remains unavailable following confirmed backend 400 failures. This is a Round 1 result only, not a full gauntlet pass or an overall readiness declaration.
@@ -0,0 +1,33 @@
+# Fatal cleanup follow-up, round 2
+
+## Review summary
+
+- Theme: architecture and patterns.
+- Model: GPT-6 Astra, parent review in the unavailable Gemini slot.
+- Scope: the three-file fatal-cleanup follow-up to `02272e0a5d`.
+- Issues found: 0.
+- Verdict: CLEAN for this bounded review, not a full gauntlet pass.
+
+## Evidence
+
+- `core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricTestArtifactTracker.scala`
+  narrows only the cleanup recovery handler to `NonFatal`. This matches bulk
+  cleanup and `FabricNotebookTests.shutdownAndCleanup`. The body handler still
+  records and immediately rethrows every throwable so ordinary cleanup failures
+  cannot mask a fatal body failure.
+- No public signature, serialized parameter, runtime setting, dependency, or
+  pipeline definition changes. The helper remains private test infrastructure.
+- `FabricTestArtifactTrackerFailureTests.scala` follows the existing private
+  test-trait structure. `FabricTestArtifactTrackerSuite` already mixes it in, so
+  the existing explicit pipeline selector includes the new regression.
+- `docs/Reference/Developer Setup.md` documents fatal propagation and retained
+  artifact tracking without claiming that later cleanup is guaranteed to run.
+- Local compile, production/test Scala style, and the tracker plus naming suites
+  passed with 47 tests. Evidence is retained locally in
+  `master-fatal-cleanup-green.log`, not published as a repository artifact.
+
+## Model limitation
+
+The Gemini slot was not executed for this follow-up. Earlier session attempts
+with the available Gemini models failed at the backend with HTTP 400 before
+review execution. This fallback does not satisfy independent Gemini coverage.
@@ -0,0 +1,164 @@
+# Review 3 — Edge Cases and Robustness
+
+- **Task:** `task-fatal-cleanup`, attempt 1 | **Round:** 3 | **Model:** claude-opus-5
+- **Branch:** master sync worktree, delta relative to HEAD `02272e0a5d`
+- **Trigger:** Copilot finding on microsoft/SynapseML#2734, comment 4066689707
+
+## Scope
+
+The three-file uncommitted follow-up only:
+`core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricTestArtifactTracker.scala`
+(one catch clause), `.../FabricTestArtifactTrackerFailureTests.scala` (+33), and
+`docs/Reference/Developer Setup.md` (+2). No wider sync or history re-audit.
+
+## Verdict
+
+**ISSUES_FOUND — 1 Low.** The reported bug is genuinely fixed and the regression is a
+real one. The Low is the mirror-image information loss the narrowed catch introduces.
+
+## Priority matrix after the change
+
+"Excluded" means anything `NonFatal` rejects: interrupts, `VirtualMachineError`,
+`ThreadDeath`, `LinkageError`, `ControlThrowable`.
+
+| body outcome | cleanup outcome | result |
+| --- | --- | --- |
+| success | success | body value returned |
+| success | NonFatal | cleanup error thrown |
+| success | excluded | cleanup throwable thrown |
+| any failure | NonFatal | body error thrown, cleanup attached as suppressed |
+| any failure | excluded | cleanup throwable thrown, **body error discarded** |
+
+The last row is correct on priority — the interrupt or fatal now wins, which is the
+point of the fix — but the body error is not merely deprioritised. Because the throw
+leaves a `finally`, the in-flight body exception is replaced outright: it is neither
+rethrown nor attached, so nothing records it.
+
+## L1 (Low) — body failure vanishes when cleanup throws an excluded throwable
+
+`withArtifact` still assigns `failure = Some(error)` in the body catch, but on the
+excluded-throwable path nothing reads it. The single production caller is
+`FabricNotebookTests.withTrackedArtifact`, where the body error is the notebook
+failure and the cleanup error is an artifact DELETE. A pipeline timeout that
+interrupts the DELETE after a notebook has already failed therefore surfaces only
+the `InterruptedException`, and the notebook failure that triggered triage is gone.
+
+This is the same class of defect the earlier rounds of this work fixed, and the
+repository already has the idiom for it: rethrow the winner with the loser attached
+under an `ne` guard, exactly as `FabricArtifactCleanup.run` and this method's own
+`NonFatal` branch do. A second catch clause preserves the new priority while keeping
+the body error visible, and the 15-case regression asserts only
+`thrown eq cleanupFailure`, so it would stay green:
+
+```scala
+case cleanupError: Throwable =>
+  failure.filterNot(_ eq cleanupError).foreach(cleanupError.addSuppressed)
+  throw cleanupError
+```
+
+If this is instead accepted as a deliberate trade-off, the new documentation sentence
+should say so, because "never suppresses an interrupt or fatal error behind a notebook
+failure" does not tell a reader that the notebook failure is then dropped.
+
+## Verified correct
+
+- The narrowed catch is the only production edit. `NonFatal` was already imported and
+  used by `cleanup()`, so no import churn and no unused symbol.
+- Identity is exact in all 15 combinations: `assert(thrown eq cleanupFailure)` pins the
+  same instance rather than the type, which is what the red log disproved.
+- The `original ne cleanupError` self-suppression guard on the `NonFatal` path is
+  untouched, so a shared instance is still never suppressed into itself.
+- Tracking and retry: `artifactIds.remove` sits after `deleteTrackedArtifact`, so any
+  throwing cleanup leaves the id tracked. The test proves the retry precisely —
+  `attempts == 1` after `withArtifact`, `2` after the first `cleanup()`, and `2` again
+  after a second `cleanup()`, so the deque drains and nothing is deleted twice.
+- `deleteTrackedArtifact` still treats `PowerBIEntityNotFound` as success, so a
+  concurrently deleted artifact is untracked rather than retried.
+- Body fatals are still recorded and rethrown by `case error: Throwable`, so a fatal
+  body error keeps priority over a `NonFatal` cleanup error.
+- Both documentation sentences are accurate: cleanup no longer hides an interrupt or
+  fatal, and an unsuccessful per-job deletion does stay tracked.
+
+## Evidence
+
+- `master-fatal-cleanup-red.log`: the new test alone, 1 run, 0 succeeded, 1 failed,
+  with `IllegalStateException: job failed was not the same instance as
+  java.lang.InterruptedException: cleanup interrupted`. A true red on old behaviour.
+- `master-fatal-cleanup-green.log`: `core/compile`, `core/Test/compile`, main and test
+  scalastyle each `Found 0 errors`, and 47 of 47 tests passing across
+  `FabricTestArtifactTrackerSuite` and `FabricArtifactNamesSuite`, with the new case
+  listed by name. 47 is the expected 46 plus one.
+- `FabricTestArtifactTrackerFailureTests.scala` is 76 lines and
+  `FabricTestArtifactTrackerSuite.scala` is unchanged at 790.
+
+## Non-blocking
+
+Tracking the same artifact id through `withArtifact` twice would leave a duplicate in
+the deque, since `ConcurrentLinkedDeque.remove` drops one occurrence. That predates
+this delta and is unaffected by it; noted only because retry behaviour was in scope.
+
+## Coverage limitations
+
+Round 1 and the parent's round 2 fallback were both GPT-family and found no concrete
+issues. No Gemini version has executed at any point in this task — every attempt has
+returned a backend HTTP 400 with zero execution — so the three-family review gate is
+**unfulfilled** and this is not a full-gauntlet green. This report covers round 3 only.
+Azure Pipelines and current-head GitHub review have not run against this delta, and the
+ports do not carry it yet, so no port proof is claimed.
+
+## L1 resolution
+
+The parent accepted L1. A separate excluded-throwable handler now attaches the
+earlier body error under an identity guard and immediately rethrows the cleanup
+throwable. This preserves fatal priority and diagnostics without wrapping it.
+The documentation states that attachment depends on throwable suppression support.
+
+The strengthened matrix failed before this handler was added:
+`master-fatal-cleanup-diagnostics-red.log` reports an empty suppressed list instead
+of the original `IllegalStateException`. The matrix now checks suppressed errors
+as well as identity. A fresh throwable probes suppression support independently,
+because `ControlThrowable` support differs across the supported Scala versions.
+A separate shared-instance interrupt case guards against self-suppression.
+Updated green validation and reviewer verification follow below.
+
+## Round 3 verification of the L1 resolution
+
+Verified against the current worktree; the original verdict and history above stand
+unchanged. **L1 is resolved. No new issue.**
+
+- **Catch order is safe.** `case NonFatal(cleanupError)` precedes
+  `case cleanupError: Throwable`, so the broad clause cannot shadow the narrow one and
+  only excluded throwables reach it. Reversing them would have silently restored the
+  original bug.
+- **Priority is unchanged; only diagnostics changed.** The excluded path still ends in
+  `throw cleanupError`, so the interrupt or fatal still wins and still replaces the body
+  error as the propagating exception. The only difference is that the body error is now
+  attached instead of vanishing, which is exactly the narrow repair L1 asked for.
+- **Identity guard is correct in both directions.**
+  `failure.filterNot(_ eq cleanupError)` yields `None` both when the body succeeded and
+  when the body threw the same instance, so `addSuppressed` is never called on self and
+  cannot raise `IllegalArgumentException`.
+- **The probe cannot contaminate the assertion.** `newCleanupFailure()` builds a fresh
+  instance, so `thrown.getSuppressed` is read from an object the probe never touched.
+  Deriving `expectedSuppressed` from `jobFailure.toSeq` also keeps the body-success rows
+  expecting an empty list, and the probe makes the matrix portable across the Scala
+  versions that differ on `ControlThrowable` suppression rather than hard-coding either.
+- **The same-instance test is a real negative control.** Without the `filterNot` guard
+  the tracker would call `addSuppressed` on self and fail with `IllegalArgumentException`
+  rather than returning the shared `InterruptedException` with an empty suppressed list.
+- **Docs are accurate.** All three sentences hold: the cleanup throwable escapes, the
+  earlier notebook failure is attached, and the qualifier "where that throwable permits
+  suppression" honestly covers categories that disable suppression instead of
+  over-promising.
+- **Evidence.** `master-fatal-cleanup-diagnostics-red.log` is a true red for the new
+  assertion — `Array() did not equal List(java.lang.IllegalStateException: job failed)`,
+  1 run, 0 succeeded, 1 failed. `master-fatal-cleanup-green-v2.log` has completed:
+  `core/compile`, `core/Test/compile`, main and test scalastyle each `Found 0 errors`,
+  `Run completed`, and 48 of 48 tests passing across both suites, with both new cases
+  listed by name. 48 is the expected 47 plus the same-instance test. The log carries no
+  `[error]` line and no `*** FAILED ***`; its nine keyword hits are test names and the
+  tracker's own expected `println` diagnostics. All three edited files were last written
+  before that run, so the log describes this source.
+
+Round 3 verification only. No Gemini version has executed at any point in this task, so
+the three-family gate remains **unfulfilled** and this is not a full-gauntlet green.
Original file line number	Diff line number	Diff line change
`@@ -29,12 +29,15 @@ private[nbtest] final class FabricTestArtifactTracker(deleteArtifact: String =>`
`29`	`29`	`deleteTrackedArtifact(artifactId)`
`30`	`30`	`artifactIds.remove(artifactId)`
`31`	`31`	`} catch {`
`32`		`- case cleanupError: Throwable =>`
	`32`	`+ case NonFatal(cleanupError) =>`
`33`	`33`	`failure match {`
`34`	`34`	`case Some(original) =>`
`35`	`35`	`if (original ne cleanupError) original.addSuppressed(cleanupError)`
`36`	`36`	`case None => throw cleanupError`
`37`	`37`	`}`
	`38`	`+ case cleanupError: Throwable =>`
	`39`	`+ failure.filterNot(_ eq cleanupError).foreach(cleanupError.addSuppressed)`
	`40`	`+ throw cleanupError`
`38`	`41`	`}`
`39`	`42`	`}`
`40`	`43`	`}`