fix: preserve Fabric cleanup safety and error propagation - #2732
Merged
Rana Singh (ranadeepsingh) merged 5 commits intoSep 22, 2026
Merged
Rana Singh (ranadeepsingh) merged 5 commits into
Rana Singh (ranadeepsingh) merged 5 commits into
Conversation
Rana Singh (ranadeepsingh)
requested a review
from Brendan Walsh (BrendanWalsh)
as a code owner
September 21, 2026 20:29
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 21, 2026 20:30
View session
This was referenced Sep 21, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unresolved critical and moderate findings block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request hardens Fabric artifact cleanup against malformed nested relation metadata and preserves cleanup diagnostics.
Changes:
- Fails closed on malformed nested relation metadata.
- Preserves cleanup exceptions and avoids self-suppression.
- Adds regression tests, documentation, and review artifacts.
Review status: one critical, one moderate, and one nit finding remain unresolved.
| File | Reviewed change |
|---|---|
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-6-claude-opus-5.md |
Review evidence |
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-5-gpt-6-astra.md |
Test review evidence |
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-4-gpt-6-astra.md |
Correctness review evidence |
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-3-claude-opus-5.md |
Robustness review evidence |
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-2-gpt-6-astra.md |
Architecture review evidence |
reviews/fabric-cleanup-relations-20260921/task-spark4-sync-20260921-attempt-1-review-1-gpt-6-astra.md |
Initial review evidence |
docs/Reference/Developer Setup.md |
Cleanup behavior documentation |
core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricTestArtifactTrackerSuite.scala |
Metadata and relation regression tests |
core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricTestArtifactTrackerFailureSuite.scala |
Repeated-exception regression tests |
core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricTestArtifactTracker.scala |
Self-suppression prevention |
core/src/test/scala/com/microsoft/azure/synapse/ml/nbtest/FabricArtifactCleanup.scala |
Relation parsing and cleanup error handling |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 21, 2026
## Summary Merge the master-first follow-up that stops cleanup after failed deletion confirmation and preserves the existing CI test selection through a private test mix-in. Carry the portable review-record path cleanup and retain the port-specific review evidence without changing runtime baselines. ## Prompting Intent The engineer requested separate Spark 4.0 and Spark 4.1 worktrees and sync PRs with relevant master changes, validated through the SynapseML PR loop. Resolve current-head findings and validate each port's exact merged source. ## Linked Sources - microsoft#2732 - microsoft#2732 (comment) - microsoft#2733 - microsoft#2734 - reviews/fabric-cleanup-relations-20260921/ - reviews/sync-20260921/ - .github/skills/synapseml-branches/SKILL.md - .github/skills/synapseml-pr-loop/SKILL.md ## Rationale Use a normal merge so the exact reviewed master fix remains reachable. Keep the shared cleanup implementation identical across branches and prove the inherited tests still run on the CI-selected suite under the port JDK. Machine-local report paths are publication metadata, not source differences; normalize them without removing historical findings or resolutions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 21, 2026
## Summary Merge the master-first follow-up that stops cleanup after failed deletion confirmation and preserves the existing CI test selection through a private test mix-in. Carry the portable review-record path cleanup and retain the port-specific review evidence without changing runtime baselines. ## Prompting Intent The engineer requested separate Spark 4.0 and Spark 4.1 worktrees and sync PRs with relevant master changes, validated through the SynapseML PR loop. Resolve current-head findings and validate each port's exact merged source. ## Linked Sources - microsoft#2732 - microsoft#2732 (comment) - microsoft#2733 - microsoft#2734 - reviews/fabric-cleanup-relations-20260921/ - reviews/sync-20260921/ - .github/skills/synapseml-branches/SKILL.md - .github/skills/synapseml-pr-loop/SKILL.md ## Rationale Use a normal merge so the exact reviewed master fix remains reachable. Keep the shared cleanup implementation identical across branches and prove the inherited tests still run on the CI-selected suite under the port JDK. Machine-local report paths are publication metadata, not source differences; normalize them without removing historical findings or resolutions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 21, 2026 21:40
View session
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 21, 2026
## Summary Restrict per-artifact cleanup recovery to NonFatal. Interrupts and fatal cleanup errors now escape even when the notebook body has already failed. Preserve the body error as suppressed where supported, with an identity guard. Add 15 throwable/body combinations and a shared-instance regression to the existing CI-selected tracker suite, and document the behavior. ## Prompting Intent The engineer requested separate master sync pull requests for spark4.0 and spark4.1 using the SynapseML PR loop. Address the verified cleanup review finding on master first, then carry the exact commit into both port branches without changing their runtime baselines. ## Linked Sources - Finding: microsoft#2734 (comment) - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Original cleanup work: microsoft#2728 - Review records: reviews/fabric-cleanup-relations-20260921/task-fatal-cleanup-attempt-1-review-*.md ## Rationale Use NonFatal for recoverable cleanup and explicitly rethrow excluded errors after retaining the original failure. Keep the body's catch-and-rethrow and ordinary cleanup-error suppression unchanged. Regressions demonstrated both fatal masking and lost body diagnostics before their fixes. The fixed master passes compile, test compile, production/test Scala style, and all 48 tracker and naming tests. Review records retain the unavailable Gemini-family gate; they do not claim complete three-family review or full Azure validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 21, 2026 22:17
View session
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 21, 2026
## Summary Merge the exact master-first fatal-cleanup correction and its regression, documentation, and review records into this Spark sync branch. ## Prompting Intent The engineer requested separate spark4.0 and spark4.1 sync worktrees and pull requests with the SynapseML PR loop. Preserve each port's runtime and compatibility differences while carrying portable fixes by normal merge. ## Linked Sources - Finding: microsoft#2734 (comment) - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Review records: reviews/fabric-cleanup-relations-20260921/task-fatal-cleanup-attempt-1-review-*.md ## Rationale Retain shared source and real merge ancestry rather than duplicate the fix or alter branch-specific settings. The master regression demonstrated the fatal-error masking bug before the NonFatal correction. This port passes compile, test compile, production/test Scala style, and 48 tracker/naming tests. Full Azure validation and the unavailable Gemini-family review gate remain explicit; this commit makes no merge-readiness claim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 21, 2026
## Summary Merge the exact master-first fatal-cleanup correction and its regression, documentation, and review records into this Spark sync branch. ## Prompting Intent The engineer requested separate spark4.0 and spark4.1 sync worktrees and pull requests with the SynapseML PR loop. Preserve each port's runtime and compatibility differences while carrying portable fixes by normal merge. ## Linked Sources - Finding: microsoft#2734 (comment) - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Review records: reviews/fabric-cleanup-relations-20260921/task-fatal-cleanup-attempt-1-review-*.md ## Rationale Retain shared source and real merge ancestry rather than duplicate the fix or alter branch-specific settings. The master regression demonstrated the fatal-error masking bug before the NonFatal correction. This port passes compile, test compile, production/test Scala style, and 48 tracker/naming tests. Full Azure validation and the unavailable Gemini-family review gate remain explicit; this commit makes no merge-readiness claim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
changed the title
Collaborator
Author
|
The previously reported replay blocker remains on current head All 3,653 reported passing tests completed successfully, including 50 tracker tests, three naming tests, 43 CPU notebooks, and four GPU notebooks. The 22 non-executed cases exactly match the audited baseline. The separate CI-helper task passed 288 tests, and all six R jobs passed. Replay uses published Spark 4.1 target
Applying the current PR afterward produces six helper files identical to #2734 head Updating |
## Summary Reject malformed nested relation metadata instead of discarding dependency edges. Preserve earlier deletion failures when a later metadata read aborts, and avoid self-suppression when cleanup reports a reused exception. Add fake-client regressions, document the accepted relation shape, and retain the review findings and their resolutions. ## Prompting Intent The engineer requested separate worktrees and sync pull requests carrying relevant master changes into spark4.0 and spark4.1, using the SynapseML PR loop. Review of that sync exposed these cleanup defects. Isolate portable fixes on master first rather than maintaining different cleanup implementations on ports. ## Linked Sources - microsoft#2728 - microsoft#2728 (comment) - AGENTS.md: cross-version changes land on master before port integration - .github/skills/synapseml-pr-loop/SKILL.md - reviews/fabric-cleanup-relations-20260921/ ## Rationale A partial dependency graph can authorize unsafe deletion, so unknown metadata must abort inventory collection. Keep outer empty/null relation semantics and valid nested GUID references. Preserve the current inventory exception while attaching earlier deletion errors, including failed job-history or schedule reads; never continue deleting after the metadata read fails. Identity guards retain real failures instead of replacing them with a self-suppression exception. This changes test infrastructure only, not public SparkML APIs, dependency pins, pipeline settings, or runtime enablement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Keep recoverable DELETE errors separate from inventory and confirmation errors. Abort the candidate loop when confirmation fails, preserving the original exception and earlier deletion failures. Cover all read sites, previous-error and repeated-error combinations, and the bounded multi-job confirmation case. Register extracted failure tests on the existing CI-selected tracker suite. Normalize machine-local paths in review records without changing their findings. ## Prompting Intent The engineer requested isolated master sync PRs for Spark 4.0 and Spark 4.1 through the SynapseML PR loop. Resolve the current-head review findings before rerunning validation, and carry portable corrections through master first. ## Linked Sources - microsoft#2732 (comment) - microsoft#2733 - microsoft#2734 - microsoft#2734 (comment) - microsoft#2734 (comment) - microsoft#2734 (comment) - microsoft#2734 (comment) - microsoft#2734 (comment) - microsoft#2734 (comment) - microsoft#2734 (comment) - pipeline.yaml: explicit FabricTestArtifactTrackerSuite selection - reviews/pr-2708/README.md: review-record publication conventions - reviews/fabric-cleanup-relations-20260921/ ## Rationale A failed post-DELETE inventory read must not permit another DELETE. Capture only actual DELETE failures as recoverable results; keep confirmation outside that boundary and use one nonfatal abort handler for the full candidate loop. An unconfirmed deletion also stops the run, while independent jobs may still be attempted after an ordinary DELETE error. Reuse the existing helper instead of adding tagged exception wrappers or disabling style rules. Mix extracted tests into the already scheduled suite so CI executes them without changing pipeline configuration. Preserve historical review content while replacing private checkout/log locations with portable evidence references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Restrict per-artifact cleanup recovery to NonFatal. Interrupts and fatal cleanup errors now escape even when the notebook body has already failed. Preserve the body error as suppressed where supported, with an identity guard. Add 15 throwable/body combinations and a shared-instance regression to the existing CI-selected tracker suite, and document the behavior. ## Prompting Intent The engineer requested separate master sync pull requests for spark4.0 and spark4.1 using the SynapseML PR loop. Address the verified cleanup review finding on master first, then carry the exact commit into both port branches without changing their runtime baselines. ## Linked Sources - Finding: microsoft#2734 (comment) - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Original cleanup work: microsoft#2728 - Review records: reviews/fabric-cleanup-relations-20260921/task-fatal-cleanup-attempt-1-review-*.md ## Rationale Use NonFatal for recoverable cleanup and explicitly rethrow excluded errors after retaining the original failure. Keep the body's catch-and-rethrow and ordinary cleanup-error suppression unchanged. Regressions demonstrated both fatal masking and lost body diagnostics before their fixes. The fixed master passes compile, test compile, production/test Scala style, and all 48 tracker and naming tests. Review records retain the unavailable Gemini-family gate; they do not claim complete three-family review or full Azure validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Check deletion immediately, then at most ten more times with 30-second waits per artifact. Preserve the existing fail-closed ordering and error handling. Inject the sleep duration so tests verify 30000 ms and a 300000 ms wait budget. Expand boundary coverage and document that HTTP request time is additional. ## Prompting Intent The engineer explicitly requested 30-second polling with a five-minute maximum, superseding the earlier one-minute request. Match the existing bounded confirmation pattern, keep one deletion call per candidate, and carry the portable master-first fix into both Spark sync pull requests. ## Linked Sources - Engineer request: "update it so it polls every 30 seconds with a max timeout of 5 mins" - Master prerequisite: microsoft#2732 - Original bounded polling: microsoft#2728 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Review records: reviews/pr-2732/task-cleanup-polling-30s-attempt-1-review-*.md ## Rationale Use eleven reads and ten waits rather than a shared or wall-clock deadline, matching the referenced per-item wait-budget semantics. Duration-aware sleeper injection tests the actual requested delays without real sleeps. Regressions cover immediate, first, middle, and final success, fresh child and parent budgets, exhaustion, inventory errors, conflicting metadata, interrupts, and protected consumers. Five unchanged failure tests move into the existing mix-in to preserve the 800-line limit and CI test registration. The old 31-read/two-second policy failed the new assertions. The final master passes core compile, test compile, production/test Scala style, and all 50 tracker/naming tests. No runtime pins, pipeline policy, or release tooling changes are included. Gemini-family review remains explicitly unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
force-pushed
the
fix/fabric-cleanup-relations-20260921
branch
from
September 22, 2026 04:06
81ccc54 to
dd33c24
Compare
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Merge the exact master-first polling update, deterministic regressions, documentation, and review records into this Spark sync. ## Prompting Intent The engineer requested 30-second deletion-confirmation intervals with a five-minute per-item wait budget. Preserve this port's runtime settings and carry portable changes through normal merge ancestry. ## Linked Sources - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Shared review-layout guidance only: microsoft#2736 - Review records: reviews/pr-2732/task-cleanup-polling-30s-attempt-1-review-*.md ## Rationale The same helper and tests run on each supported Scala baseline. This port passes core compile, test compile, production/test Scala style, and all 50 tracker/naming tests. Polling performs one immediate read and at most ten 30-second waits per item, with request time additional, and never resends DELETE. This merge preserves runtime pins and existing Fabric enablement. Previous-head Azure results do not validate this new head; fresh remote checks remain required. The Gemini-family review limitation stays visible. Copy only the six-line shared review-layout guidance from latest master so AGENTS.md and CONTRIBUTING.md stay identical across the task branches. The remaining newly merged CI changes are not imported without approval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Merge the exact master-first polling update, deterministic regressions, documentation, and review records into this Spark sync. ## Prompting Intent The engineer requested 30-second deletion-confirmation intervals with a five-minute per-item wait budget. Preserve this port's runtime settings and carry portable changes through normal merge ancestry. ## Linked Sources - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 sync: microsoft#2734 - Shared review-layout guidance only: microsoft#2736 - Review records: reviews/pr-2732/task-cleanup-polling-30s-attempt-1-review-*.md ## Rationale The same helper and tests run on each supported Scala baseline. This port passes core compile, test compile, production/test Scala style, and all 50 tracker/naming tests. Polling performs one immediate read and at most ten 30-second waits per item, with request time additional, and never resends DELETE. This merge preserves runtime pins and existing Fabric enablement. Previous-head Azure results do not validate this new head; fresh remote checks remain required. The Gemini-family review limitation stays visible. Copy only the six-line shared review-layout guidance from latest master so AGENTS.md and CONTRIBUTING.md stay identical across the task branches. The remaining newly merged CI changes are not imported without approval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 22, 2026 04:07
View session
## Summary Use one exception boundary for smoke and notebook job waits. Restore interrupt status, rethrow the original interrupt or fatal error, and retain notebook context for ordinary failures. Add deterministic regressions and document the contract. Move cleanup review history under its numbered PR directory and correct a misleading timing statement while preserving its original wording as explicitly labeled historical feedback. ## Prompting Intent Complete the requested Spark sync PR loop and address all current-head review findings without changing runtime pins, CI policy, or Fabric enablement. Preserve the requested 30-second, per-item five-minute waiting budget. ## Linked Sources - Smoke wait review: microsoft#2734 (comment) - Report placement review: microsoft#2734 (comment) - Timing wording review: microsoft#2732 (comment) - Master prerequisite: microsoft#2732 - Review records: reviews/pr-2732/task-job-wait-errors-attempt-1-review-*.md ## Rationale Share the notebook path's existing exception policy rather than maintaining a second catch-all in smoke tests. The original smoke await/assert semantics and resource lifecycle remain unchanged. After behavior-preserving extraction, the previous catch-all failed the interrupt and fatal-error regressions. Master compilation, test compilation, both Scala style checks, and all 53 tracker/naming tests pass. Tests use an actual interrupted Await without contacting Fabric. No public SparkML signature or serialized parameter changes. Gemini-family review remains unavailable rather than being reported as passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 22, 2026 05:11
View session
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Carry the reviewed master-first job-wait correction and review-record cleanup into this port by a normal merge. Place this port's own historical reports under its numbered PR directory. ## Prompting Intent Resolve current-head sync review findings while retaining the port runtime, CI policy, disabled Fabric E2E, and requested deletion-confirmation cadence. ## Linked Sources - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 review: microsoft#2734 (comment) - Review records: reviews/pr-2732/task-job-wait-errors-attempt-1-review-*.md ## Rationale The master prerequisite was rebased after master acquired approval-gated CI changes. Apply its exact follow-up patch to the previously synced master baseline, then merge that portable branch into each port. This retains merge ancestry without importing unapproved pipeline changes. Shared helper content must match the master fix, and each port must pass compilation, test compilation, both Scala style checks, and all 53 tracker/naming tests before this commit. Historical report names, reviewed revisions, findings, and resolutions remain available in numbered PR directories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Carry the reviewed master-first job-wait correction and review-record cleanup into this port by a normal merge. Place this port's own historical reports under its numbered PR directory. ## Prompting Intent Resolve current-head sync review findings while retaining the port runtime, CI policy, disabled Fabric E2E, and requested deletion-confirmation cadence. ## Linked Sources - Master prerequisite: microsoft#2732 - Spark 4.0 sync: microsoft#2733 - Spark 4.1 review: microsoft#2734 (comment) - Review records: reviews/pr-2732/task-job-wait-errors-attempt-1-review-*.md ## Rationale The master prerequisite was rebased after master acquired approval-gated CI changes. Apply its exact follow-up patch to the previously synced master baseline, then merge that portable branch into each port. This retains merge ancestry without importing unapproved pipeline changes. Shared helper content must match the master fix, and each port must pass compilation, test compilation, both Scala style checks, and all 53 tracker/naming tests before this commit. Historical report names, reviewed revisions, findings, and resolutions remain available in numbered PR directories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Rana Singh (ranadeepsingh)
merged commit Sep 22, 2026
0a7fdaf
into
microsoft:master
82 of 83 checks passed
Rana Singh (ranadeepsingh)
added a commit
that referenced
this pull request
Sep 22, 2026
## Summary Keep recoverable DELETE errors separate from inventory and confirmation errors. Abort the candidate loop when confirmation fails, preserving the original exception and earlier deletion failures. Cover all read sites, previous-error and repeated-error combinations, and the bounded multi-job confirmation case. Register extracted failure tests on the existing CI-selected tracker suite. Normalize machine-local paths in review records without changing their findings. ## Prompting Intent The engineer requested isolated master sync PRs for Spark 4.0 and Spark 4.1 through the SynapseML PR loop. Resolve the current-head review findings before rerunning validation, and carry portable corrections through master first. ## Linked Sources - #2732 (comment) - #2733 - #2734 - #2734 (comment) - #2734 (comment) - #2734 (comment) - #2734 (comment) - #2734 (comment) - #2734 (comment) - #2734 (comment) - pipeline.yaml: explicit FabricTestArtifactTrackerSuite selection - reviews/pr-2708/README.md: review-record publication conventions - reviews/fabric-cleanup-relations-20260921/ ## Rationale A failed post-DELETE inventory read must not permit another DELETE. Capture only actual DELETE failures as recoverable results; keep confirmation outside that boundary and use one nonfatal abort handler for the full candidate loop. An unconfirmed deletion also stops the run, while independent jobs may still be attempted after an ordinary DELETE error. Reuse the existing helper instead of adding tagged exception wrappers or disabling style rules. Mix extracted tests into the already scheduled suite so CI executes them without changing pipeline configuration. Preserve historical review content while replacing private checkout/log locations with portable evidence references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
that referenced
this pull request
Sep 22, 2026
## Summary Restrict per-artifact cleanup recovery to NonFatal. Interrupts and fatal cleanup errors now escape even when the notebook body has already failed. Preserve the body error as suppressed where supported, with an identity guard. Add 15 throwable/body combinations and a shared-instance regression to the existing CI-selected tracker suite, and document the behavior. ## Prompting Intent The engineer requested separate master sync pull requests for spark4.0 and spark4.1 using the SynapseML PR loop. Address the verified cleanup review finding on master first, then carry the exact commit into both port branches without changing their runtime baselines. ## Linked Sources - Finding: #2734 (comment) - Master prerequisite: #2732 - Spark 4.0 sync: #2733 - Spark 4.1 sync: #2734 - Original cleanup work: #2728 - Review records: reviews/fabric-cleanup-relations-20260921/task-fatal-cleanup-attempt-1-review-*.md ## Rationale Use NonFatal for recoverable cleanup and explicitly rethrow excluded errors after retaining the original failure. Keep the body's catch-and-rethrow and ordinary cleanup-error suppression unchanged. Regressions demonstrated both fatal masking and lost body diagnostics before their fixes. The fixed master passes compile, test compile, production/test Scala style, and all 48 tracker and naming tests. Review records retain the unavailable Gemini-family gate; they do not claim complete three-family review or full Azure validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
that referenced
this pull request
Sep 22, 2026
## Summary Check deletion immediately, then at most ten more times with 30-second waits per artifact. Preserve the existing fail-closed ordering and error handling. Inject the sleep duration so tests verify 30000 ms and a 300000 ms wait budget. Expand boundary coverage and document that HTTP request time is additional. ## Prompting Intent The engineer explicitly requested 30-second polling with a five-minute maximum, superseding the earlier one-minute request. Match the existing bounded confirmation pattern, keep one deletion call per candidate, and carry the portable master-first fix into both Spark sync pull requests. ## Linked Sources - Engineer request: "update it so it polls every 30 seconds with a max timeout of 5 mins" - Master prerequisite: #2732 - Original bounded polling: #2728 - Spark 4.0 sync: #2733 - Spark 4.1 sync: #2734 - Review records: reviews/pr-2732/task-cleanup-polling-30s-attempt-1-review-*.md ## Rationale Use eleven reads and ten waits rather than a shared or wall-clock deadline, matching the referenced per-item wait-budget semantics. Duration-aware sleeper injection tests the actual requested delays without real sleeps. Regressions cover immediate, first, middle, and final success, fresh child and parent budgets, exhaustion, inventory errors, conflicting metadata, interrupts, and protected consumers. Five unchanged failure tests move into the existing mix-in to preserve the 800-line limit and CI test registration. The old 31-read/two-second policy failed the new assertions. The final master passes core compile, test compile, production/test Scala style, and all 50 tracker/naming tests. No runtime pins, pipeline policy, or release tooling changes are included. Gemini-family review remains explicitly unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Merge master 0a7fdaf into the existing Spark 4.1 sync. Import the portable test cleanup and conservative notebook selection changes while retaining the Spark 4.1 runtime and Python compatibility. ## Prompting Intent The user requested updating both existing Spark sync PRs with the latest master changes, maximizing master compatibility while honoring Spark- and Python-specific differences. ## Linked Sources - Sync PR: microsoft#2734 - Cleanup prerequisite: microsoft#2732 - Test retirement: microsoft#2735 - Notebook selection: microsoft#2736 - Master: microsoft@0a7fdaf - Review evidence: reviews/pr-2734/task-latest-master-20260922-attempt-1-review-*.md ## Rationale Use a normal merge to retain master's ancestry, including the landed cleanup fixes already present in this PR. Accept the obsolete Python runner deletion rather than preserving its irrelevant Scala-version edit. Keep Fabric E2E disabled and assert that port requirement in the imported selector wiring test. The selector itself matches master. Production code, dependency pins, runtime profiles, streaming scheduling, and existing Spark/Python adaptations remain unchanged from the previously validated port head. Strengthen the full-test override regression with valid skippable merge metadata and the non-PR bypass regression with a detection tripwire. Document the disabled Fabric boundary accurately. These review fixes do not change selector behavior or enable unsupported runtime jobs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Sep 22, 2026
## Summary Merge master 0a7fdaf into the existing Spark 4.0 sync. Import the portable test cleanup and conservative notebook selection changes while retaining the Spark 4.0 runtime and Python compatibility. ## Prompting Intent The user requested updating both existing Spark sync PRs with the latest master changes, maximizing master compatibility while honoring Spark- and Python-specific differences. ## Linked Sources - Sync PR: microsoft#2733 - Cleanup prerequisite: microsoft#2732 - Test retirement: microsoft#2735 - Notebook selection: microsoft#2736 - Master: microsoft@0a7fdaf - Review evidence: reviews/pr-2733/task-latest-master-20260922-attempt-1-review-*.md ## Rationale Use a normal merge to retain master's ancestry, including the landed cleanup fixes already present in this PR. Accept the obsolete Python runner deletion rather than preserving its irrelevant Scala-version edit. Keep Fabric E2E disabled and assert that port requirement in the imported selector wiring test. The selector itself matches master. Production code, dependency pins, runtime profiles, streaming scheduling, and existing Spark/Python adaptations remain unchanged from the previously validated port head. Strengthen the full-test override regression with valid skippable merge metadata and the non-PR bypass regression with a detection tripwire. Document the disabled Fabric boundary accurately. These review fixes do not change selector behavior or enable unsupported runtime jobs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Summary
Make Fabric test cleanup safer when metadata is incomplete, deletion is delayed,
or a job fails. This changes test infrastructure only, not public SparkML APIs,
runtime versions, dependencies, or Fabric enablement.
What changes
The five-minute waiting budget excludes request time. Polling never repeats DELETE.
Validation and remaining gates
53 targeted tests pass on master and both Spark ports. Both sync PRs also pass
full Azure CI. No live Fabric deletion testing is claimed.
The master Azure build
passed 65 jobs but failed compatibility replay
because Spark 4.1 lacks the cleanup baseline. Updating the prerequisite metadata
needs approval. Human approval is also required; Gemini review was unavailable.
Land this prerequisite before #2733 and #2734.