`IO#syncStep` drops `uncancelable` and `onCancel` from the returned `IO`, so `Resource` release is skipped

[tobiajo]

When IO#syncStep stops at an async boundary, the IO it returns in Left has lost the uncancelable and onCancel it was wrapped in. Canceling it skips finalizers and interrupts masked regions. The visible symptom is that Resource.use no longer releases.

Reproduced on cats-effect 3.7.1, Scala 2.13.18, JVM.

Reproduction

//> using scala 2.13.18
//> using dep org.typelevel::cats-effect:3.7.1
import cats.effect._, unsafe.implicits.global
import scala.concurrent.duration._, scala.util.Try

object Repro extends App {
  var released = false
  val effect = Resource.make(IO.unit)(_ => IO { released = true }).use(_ => IO.never)

  val result = Try {
    effect.syncStep(Int.MaxValue).unsafeRunSync() match {
      case Right(a)   => a
      case Left(rest) => rest.timeout(50.millis).unsafeRunSync()
    }
  }

  println(s"result = $result, released = $released")
  // result = Failure(java.util.concurrent.TimeoutException: 50 milliseconds), released = false
}

Running effect.timeout(50.millis).unsafeRunSync() directly, without the syncStep, prints released = true.

The timeout is not the point. I opened #4686 with three pending IOSuite tests: a Resource.use canceled from inside and from another fiber, both skipping the release, and an IO.uncancelable whose returned IO no longer blocks a cancel.

Cause

SyncStep.interpret walks the IO node by node until it reaches one it cannot run in G. When G's root cancel scope is Uncancelable, it does not stop at Uncancelable and OnCancel nodes but walks into them (IO.scala#L2390-L2398):

// walk inside: unwrap the region, replace Poll with identity, keep going
case IO.Uncancelable(body, _) if G.rootCancelScope == CancelScope.Uncancelable =>
  val ioa = body(new Poll[IO] { def apply[C](ioc: IO[C]): IO[C] = ioc })
  interpret(ioa, limit, stepsUntilDefer)

// walk past: keep the effect, drop the finalizer
case IO.OnCancel(ioa, _) if G.rootCancelScope == CancelScope.Uncancelable =>
  interpret(ioa, limit, stepsUntilDefer)

// stop: return the node as is
case _ =>
  G.pure(Left(io))

This is fine as long as the walk stays inside G: an uncancelable G cannot actually be canceled, so the two wrappers make no difference there. It stops being fine when the walk hits an async boundary. The inner IO is then returned as Left and the wrappers are not put back, so it runs on a cancelable runtime again without its mask and without its finalizer. Resource.use is built from exactly these two, which is why its release is what gets dropped.

The two cases came in with #3064 and #3065 (v3.3.14, v3.4.0), to let a Resource.allocated with a synchronous acquire be stepped through. The shortcut was noticed in review (discussion).

Nothing caught it because IO#syncStep always uses SyncIO, which is uncancelable, while AsyncLaws.syncStepIdentity runs with G = F = IO, which is cancelable, so the laws never reach these branches. The IOSuite tests for the two cases only check that the prefix completes.

[tobiajo]

The only workaround I found to use syncStep safely is to provide the interpreter a custom Sync[SyncIO] with rootCancelScope = CancelScope.Cancelable. It then stops at Uncancelable and OnCancel instead of walking into them, and the returned IO keeps both.

[djspiewak]

Sneaky! I think we could solve this by passing down the finalizers we've elided as we recursively traverse. These would be ignored unless we hit a Left within the bounds of an OnCancel, at which point we would add it back. I think we would need to something similar with uncancelable.

More generally, I think this problem will occur with any non-algebraic suspension which is not interpretable in the target monad.

[tobiajo]

Yes, for OnCancel I see that we can simply rewrap the Left as rest.onCancel(fin).

Uncancelable with its Poll makes it much more complicated. Just rewrapping as Uncancelable(_ => rest) would obviously be wrong: it masks the whole returned IO. The current identity Poll also loses track of where unmasking was done.

Instead I think we must mark the unmasking and bind it to the poll in the rebuilt Uncancelable(poll => ...). As far as I can tell that needs new unmasking/binding constructs the IOFiber run loop has to understand, which is beyond me, so exactly how I cannot say.

The simpler option is to not walk into Uncancelable at all, but that gives up #3064 and stepping Resource.

[tobiajo]

Instead I think we must mark the unmasking and bind it to the poll in the rebuilt Uncancelable(poll => ...). As far as I can tell that needs new unmasking/binding constructs the IOFiber run loop has to understand, which is beyond me, so exactly how I cannot say.

I prototyped this, but as I said, the run loop part is out of my full understanding. Added two new internal IO case classes: UnmaskSyncStep marks where poll was applied during interpretation, and BindPoll binds it to the real poll in the rebuilt Uncancelable.

case IO.Uncancelable(body, event) if G.rootCancelScope == CancelScope.Uncancelable =>
  val poll = new Poll[IO] { self =>
    def apply[C](ioc: IO[C]): IO[C] = IO.Uncancelable.UnmaskSyncStep(ioc, self)
  }

  val next =
    try body(poll)
    catch { case t if UnsafeNonFatal(t) => IO.raiseError[B](t) }

  // only our own rebuilt body returns a `BindPoll`: keep the poll it already binds and
  // drop the one just handed out, rather than chaining one onto the other on every step
  val (ioa, unbound) = next match {
    case IO.Uncancelable.BindPoll(ioa, unbound, _) => (ioa, unbound)
    case _ => (next, poll)
  }

  interpret(ioa, limit, stepsUntilDefer - 1).map {
    case Left(rest) =>
      Left(
        IO.Uncancelable(bound => IO.Uncancelable.BindPoll(rest, unbound, bound), event))
    case r @ Right(_) => r
  }

case IO.Uncancelable.UnmaskSyncStep(ioa, unbound) =>
  interpret(ioa, limit, stepsUntilDefer - 1).map {
    case Left(rest) => Left(IO.Uncancelable.UnmaskSyncStep(rest, unbound))
    case r @ Right(_) => r
  }

case IO.OnCancel(ioa, fin) if G.rootCancelScope == CancelScope.Uncancelable =>
  interpret(ioa, limit, stepsUntilDefer - 1).map {
    case Left(rest) => Left(rest.onCancel(fin))
    case r @ Right(_) => r
  }