fix(#525): a cancelled import reported .importFailed when the break landed in the transfer

[gsdali]

What

A cancelled import could report ImportError.importFailed instead of ImportError.cancelled,
depending on which phase the cancellation landed in. Filed from a 1-in-9 flake in #300's own
regression test; reachable by any caller whose deadline expires early.

Root cause

OCCTBridge_IO.mm set *outCancelled only at its own explicit UserBreak() checkpoints. Those
are not the only way out of these functions:

exit	when	reported as (before)
TransferRoots(...) == 0	break during the transfer	.importFailed
null shape / non-Done status	break mid-transfer	.importFailed
catch (...)	break raised inside OCCT	.importFailed

Measured on the #300 fixture: cancel-on-first-poll and cancel-at-fraction-0.25 both returned
.importFailed; cancel-at-0.60 (inside the repair half) returned .cancelled. The flaky test
derived its deadline from a wall-clock measurement of a previous import, so machine load decided
which of those it hit.

The header has documented the intended contract since v0.168.0 — "the *Progress entry points
return NULL and set *outCancelled=true; if the import otherwise fails, *outCancelled stays
false" — so this is the implementation catching up to it, not an API change.

Second defect, found while probing the first

BridgeProgressIndicator::UserBreak() re-asked the caller at every checkpoint and believed the
latest answer. A caller that answers true once — a one-shot flag, an already-consumed
Task.isCancelled — had that answer overwritten: OCCT aborted the phase, the next poll said "no
break", and the partially-repaired shape came back as a success. Measured: 20,197 polls and a
returned shape, against 20,436 for an uncancelled run.

ImportProgress.shouldCancel documents the opposite ("the loader throws ImportError.cancelled
on the next boundary after this returns true"), so the break is now latched — std::atomic<bool>,
since OCCT documents UserBreak() as callable concurrently.

Fix

• UserBreak() latches the first true; Cancelled() reads the latch without re-polling.
• Every failure return below the indicator's construction reports cancellation from that latch —
  including the catch (...) handlers, which needed the indicator hoisted out of the try.
• Applied to all twelve *Progress entry points, not only the two robust importers: same shape,
  same defect.

Bridge-only: no kernel patch, no OCCT.xcframework rebuild. OCCTBridge.xcframework needs a
republish at release time since OCCTBridge_IO.mm changed.

Tests

Both #300 regression tests are rewritten off the clock:

• repair is inside the caller's range — checked by the silence that would follow the last
  progress report if it were not. Measured 1.3% (STEP) / 3.4% (IGES) of the call with the fix,
  35–40% with the Sweep: ShapeFix/Sewing run outside the caller's progress range in the Robust *Progress import paths (#286 follow-up) #300 defect reintroduced. A ratio taken within one call, so a slow machine
  stretches both halves of it.
• a cancellation there stops the repair — checked against the uncancelled run's poll count, a
  count of work items rather than a duration (measured identical across runs: 20,436 STEP /
  38,817 IGES).

Step names cannot stand in for the phase, tempting as they look: both readers run a
ShapeFix_Shape of their own during the transfer, so Fixing face / Fixing edge /
Update tolerances are already being reported from fraction ~0.09.

New suite CancellationReportingTests (#525): transfer-phase cancellation on loadRobust /
loadIGESRobust / loadSTEP, and the one-shot canceller.

Each new test was verified to fail against the defect it covers, re-injected one at a time:

injection	fails
transfer exit drops the cancel flag	both transfer-phase tests, with the exact .importFailed message from the issue
UserBreak() stops latching	the one-shot test only
#300 defect restored (transfer takes the whole range, repair gets none)	both #300 tests, on the tail ratio (0.35 / 0.40 vs a 0.25 threshold)

Previously flaky suites: 12/12 clean. Full swift test: see below.

Closes #525

---

🤖 Generated with Claude Code

[gsdali]

Review: #582 — fix(#525): a cancelled import reported .importFailed when the break landed in the transfer

Note: this PR targets main directly, not refactor/381-pass1b — it's outside the Pass 1b initiative.

Overview

Two related defects in OCCTBridge_IO.mm's cancellation handling, found while chasing a 1-in-9 flake in #300's own regression test. (1) *outCancelled was only set at the bridge's own explicit UserBreak() checkpoints — a break landing inside TransferRoots (reported as zero transferred roots), inside a null-shape/non-Done exit, or as a caught exception all fell through to reporting .importFailed for a call the caller had explicitly cancelled. (2) BridgeProgressIndicator::UserBreak() re-polled the caller at every checkpoint and trusted only the latest answer, so a one-shot true (a consumed Task.isCancelled, a fire-once flag) could be overwritten by a later false poll, silently turning a cancelled call into a reported success. Both are fixed by latching the break (std::atomic<bool>) and routing every failure exit in all twelve *Progress entry points through a single setCancelOut that reads the latch instead of re-polling.

Correctness — verified directly against OCCT source, not the PR's account of it

• UserBreak()'s documented concurrency contract is exactly as claimed. Checked Message_ProgressIndicator.hxx: "This method can be called concurrently, thus implementation should be thread-safe" — confirms std::atomic<bool> (rather than a plain bool) is the right tool here, not defensive overkill.
• "A break during the transfer leaves zero roots transferred" is exactly right, verified at the actual loop. XSControl_Reader::TransferRoots (XSControl_Reader.cxx:273-296, the base class both STEPControl_Reader and IGESControl_Reader inherit): for (size_t i = 1; i <= theroots.Size() && aProgressScope.More(); i++). If the break is observed before the loop's first iteration, aTransferredCount never leaves its initial 0 — exactly the claimed mechanism.
• Went beyond the diff hunks to check exit-path completeness across the full file (fetched the PR's complete OCCTBridge_IO.mm, not just the changed lines) — every repair-phase exit in OCCTImportSTEPRobustProgress/OCCTImportIGESRobustProgress (the SOLID branch's fixer.Perform, the COMPOUND/SHELL/FACE branch's sewing-then-fix, and the fallback branch) has its own UserBreak() check routed through setCancelOut, matching the PR's claim that the repair phase is covered, not just the transfer.
• One apparent gap, checked and resolved as a non-issue rather than left unexamined: OCCTImportSTEPProgress (the plain, non-robust importer)'s if (shape.IsNull()) return nullptr; has no setCancelOut call. This looks like a miss at first glance, but the line immediately above it is if (indicator->UserBreak()) { setCancelOut(...); return nullptr; } — and since UserBreak() (not Cancelled()) is called there, it's a genuine re-poll of the caller, not just a latch read. If that poll returns false, the caller has just affirmatively said "don't cancel," so any subsequent shape.IsNull() is a real transfer failure, not a swallowed cancellation — there's no polling boundary between that check and OneShape() where a break could land undetected. The asymmetry with the robust importers (which do check UserBreak() after OneShape() too) is cosmetic, not a correctness gap.
• The memory_order_relaxed choice on myBroken is the textbook-correct minimal ordering for a flag with no accompanying data to publish. Cancelled() is only ever read after the OCCT call that could set the flag has already returned control to the calling thread (whatever internal synchronization that call used to join its own workers already establishes the needed visibility) — there's no case here where relaxed ordering could let a Cancelled() read miss a UserBreak() write from a genuinely concurrent poll that matters to the caller.
• The setCancelOut(bool*, const handle&) signature tightening to const is correctly justified — it now calls the newly-const Cancelled() rather than the non-const UserBreak(), so taking the handle by const reference is a real tightening, not just cosmetic.

Test coverage

• Both new ImmediateCanceller tests target the exact defect table row from the issue — cancel on the very first poll, before TransferRoots can transfer anything, is precisely the "zero roots" pre-fix .importFailed path.
• OneShotCanceller is the right shape to prove the latch, not just its presence: it deliberately waits for fraction >= 0.6 (inside the repair phase) before firing once, then never again — this is the fixture that would have silently regressed to a returned shape under the old re-polling UserBreak(), and the assertion (ImportError.cancelled thrown, canceller.fired == true) is specific enough to fail if the latch were ever removed.
• The Sweep: ShapeFix/Sewing run outside the caller's progress range in the Robust *Progress import paths (#286 follow-up) #300 test rewrite from wall-clock deadlines to poll-count comparisons is a genuine robustness improvement, not just a refactor for its own sake. The PR states the old tests were themselves the source of the flake being investigated (a 0.75 × previous-run-wall-clock deadline landing in different phases depending on machine load) — replacing a duration comparison with a work-item count (canceller.polls < baseline.polls) removes exactly that nondeterminism, and the new BaselineProgress/expectRangeCoversWholeCall helper's tail-ratio check (tail/total < 0.25) is unit-free, so it doesn't reintroduce a different absolute-timing fragility.
• Proved rather than assumed, per this project's established discipline: the PR states each new test was verified to fail against its specific defect, re-injected one at a time (transfer-exit dropping the flag, UserBreak() un-latched, the Sweep: ShapeFix/Sewing run outside the caller's progress range in the Robust *Progress import paths (#286 follow-up) #300 defect itself restored) — matching this project's "inject the defect, confirm the test catches it" bar. I did not independently re-run these against a reverted bridge, but the specificity of which tests are claimed to fail for which injection (only the one-shot test fails when un-latching, not the transfer-phase tests) is exactly the kind of claim that's easy to get wrong if fabricated and hard to get right by accident — a point in its favor.
• The Fixing face/Fixing edge progress-name caveat is a genuine, non-obvious finding, not a throwaway comment: both readers run their own ShapeFix_Shape during the transfer (at fraction ~0.09, per the PR), so a test discriminating "transfer vs. repair phase" by progress step name rather than fraction would be measuring the wrong thing — worth having in the test file as a warning against a plausible-looking alternative that would be silently wrong.

Documentation

• OCCTBridge.h's cancellation contract comment, ImportProgress.swift's protocol doc, Shape.swift's loadRobust doc, docs/reference/Concurrency.md, and the CHANGELOG all state the same two guarantees (whichever-phase → .cancelled; one true is enough) in consistent language — checked that none of the five drifted into a subtly different claim.
• The ImportProgress.swift code example (Cancel class using NSLock + a plain Bool) is a good choice specifically because it's a one-shot-capable pattern — it demonstrates the exact guarantee being documented (a canceller that only needs to be asked once) rather than a generic always-current-state canceller that wouldn't exercise the latch at all.

Risk / merge notes

• Bridge-only change (no kernel patch, no OCCT.xcframework rebuild) — but OCCTBridge.xcframework needs republishing at release time since OCCTBridge_IO.mm changed, which the PR states explicitly.
• This PR targets main, not refactor/381-pass1b — worth confirming that's intentional before merging (it is a #525 cancellation-contract fix, independent of the Pass 1b duplication-audit work, so a direct-to-main target seems appropriate rather than an oversight, but flagging since every other currently open PR in this repo targets refactor/381-pass1b).
• No apparent file-overlap risk against the Pass 1b PRs — OCCTBridge_IO.mm, ImportProgress.swift, and the OCCTIOTests.swift cancellation suites are untouched by any of refactor(#562): five knot-splitting spellings collapse onto two, and the "weaker" duplicate was the stronger one #589–fix(#603): a whole ellipse stops measuring 1.7% longer than it is #608.
• Twelve call sites is a wide surface for one PR to touch mechanically; worth a final skim at merge time that no thirteenth *Progress entry point was missed (I did not independently enumerate all *Progress bridge functions against the twelve named in the PR body, only verified the ones shown in the diff).

Verdict

Approve. The two OCCT-level claims underpinning this fix — UserBreak()'s documented concurrent-call contract and TransferRoots's zero-count behavior on an early break — check out exactly against the pinned headers and kernel source. A full read of the post-fix file (not just the diff hunks) confirms every repair-phase exit is covered and that the one exit that looks unguarded at first glance is provably safe due to an immediately preceding re-poll. Test coverage is specific to each defect and correctly removes wall-clock fragility from the tests that originally exposed the bug. No blocking correctness, convention, or coverage issues.