📊 Code Coverage Report

Current PR Coverage

Overall Coverage: 🟠 57.9%

• 10144 / 17532 expressions covered

Overall Summary

Coverage by Module

🟢 ≥80% 🟡 ≥60% 🟠 ≥40% 🔴 <40%

📈 Coverage Comparison vs. Main

➡️ Coverage unchanged at 57.9%

Update from downstream: this completely fixed lag time with increased concurrent startup in our test suite, ~20x performance improvement with 16 cores (and actually completes successfully instead of hanging forever with 32 cores).

Could you explain how the previous use of TVar (HashMap TaskToken (Async ())) was causing livelock and how that explains the CI issues? I don't see any places where it was retrying. We were seeing what looked like test cases using database connections from other test cases, but I don't see the connection to this patch.

Temporal activities using stale database connections was fixed in this PR, which ensured that all workflows received the shutdown message. I believe the behavior we were observing was something like:

1. We have three workflows running, the second of which is uninterruptibly frozen or deadlocked
2. We shutdown, which does forM_ workers cancel.
3. The first worker receives the cancel, shuts down, and cancel returns, allowing us to go to the next worker.
4. The second worker receives the cancel, but is frozen for some reason. cancel never returns.
5. A timeout is sent the thread doing forM_ workers cancel, which interrupts it and causes it to exit.
6. Our third worker is never canceled and continues working!

By doing concurrent shutdown, we ensure that each worker at least receives the message to cancel. This appears to have completely solved the problem of "Temporal processes talking to databases after their tests have exited," but replaced it with the underlying problem: "Some temporal processes/threads/workflows/whatever are hanging forever and can't be canceled."

As for this change - let me start with a brief overview on how STM works. When you do atomically $ stuff, you open a transaction. Every read and write to a variable is recorded in a log. Before committing, that log is checked against other logs. If another transaction has modified any of those variables, then the work is restarted - an implicit retry is built in to every readTVar and writeTVar that is triggered by another thread successfully writing against those variables.

Let's consider this bit of code:

join $ atomically $ do
              currentWorkflows <- readTVar worker.runningWorkflows
              writeTVar worker.runningWorkflows $ HashMap.delete runId_ currentWorkflows

This happens in removeEvictedWorkflowInstances. This takes a lock on the worker.runningWorkflows :: TVar (HashMap RunId WorkflowInstance). However, we only actually care about the runId_ in there. The stm-containers version allows the update to be tightly scoped around the runId_, so other access to that container doesn't block anyone else.

Another potential example:

  liftIO $ atomically $ do
    workflows <- readTVar worker.runningWorkflows
    case HashMap.lookup r workflows of
      Nothing -> do
        let workflows' = HashMap.insert r inst workflows
        writeTVar worker.runningWorkflows workflows'
        pure inst
      Just existingInstance -> do
        writeTVar worker.runningWorkflows workflows
        pure existingInstance

Here we are reading from and writing to the variable. If we have several of these transactions all attempting to complete, the TVar (HashMap _ _) structure makes livelock and transaction invalidation vastly more likely (ie every single transaction will invalidate every other transaction). With stm-containers, you only get transaction invalidation if you're operating on the same r key to the map.

There isn't exactly a "smoking gun" here on what transaction or combination of transactions caused the problem. There's the application of a simple rule ("TVar (Map _ _) is always wrong") and a simple fix, and now the problem is gone.

With concurrency=32 how many temporal threads did we have contending for the one shared map? And roughly how often were they updating it? I believe STM guarantees that one contending transaction will always commit, so there would have to be a lot of contention for this to become a problem

I don't have any idea, and we don't have good tooling to understand or diagnose things at this level. You're welcome to put further effort into the investigation here - I wouldn't be surprised if only one of these TVars were the real problem and the other is a fluke - but IMO the point of "good practices" and "antipatterns" is not that they always blow up, but that they can blow up in spooky ways, and the effort of "not doing the antipattern" is always less than the cost of "the antipattern exploded."

IMO the point of "good practices" and "antipatterns" is not that they always blow up, but that they can blow up in spooky ways, and the effort of "not doing the antipattern" is always less than the cost of "the antipattern exploded."

A TVar containing a map is not an anti-pattern. Using one in a situation where updates to keys are highly contended can lead to sub-optimal performance and yes something like stm-containers does improve it, but I'm not convinced that kind of situation is happening here

A TVar containing a map is an easily avoidable footgun, where all the available alternatives are better. If you don't need transactional behavior (ie all access to the Map is through readTVarIO), then you don't need a TVar at all and an IORef is sufficient and doesn't let you shoot yourself in the foot. If you need transactional behavior, then stm-containers actually works and does not livelock. TVar (Map _ _) is worse in every way, and I've solved several serious problems by replacing them, so I'm pretty content to call them an antipattern.

but I'm not convinced that kind of situation is happening here

I'd be curious to hear your hypothesis on a) what was going on and b) why this patch fixed it.

I'd be curious to hear your hypothesis on a) what was going on and b) why this patch fixed it.

What evidence do we have that this fixed it? Did we put CI back to the original >16 concurrency with this patch? Didn't we also do other mitigations to get people unblocked on their day-to-day work?

I'm just trying to get an understanding of why this patch would fix things, because I find it surprising, and I was hoping you'd have some insight.

AFAICT the only place where the use of stm-containers actually can make things faster in this patch is upsertWorkflowInstance. Is that function really hammered in CI?

There's also the unrelated change to use concurrent cancels in execute when it polls for shutdown. Maybe that's a more likely explanation, based on your explanation in this earlier comment?

If you don't need transactional behavior (ie all access to the Map is through readTVarIO), then you don't need a TVar at all and an IORef is sufficient and doesn't let you shoot yourself in the foot.

If we're talking about footguns then wouldn't you say using an IORef is an even bigger one due to race conditions?

If you need transactional behavior, then stm-containers actually works and does not livelock.

* assuming your workload is mostly updates at keys, not deletions or insertions. You still have to think about the problem and understand it before you choose a solution. Often a TVar containing a map will be just as good and much simpler since you're probably already using vanilla Maps anyway

What evidence do we have that this fixed it? Did we put CI back to the original >16 concurrency with this patch? Didn't we also do other mitigations to get people unblocked on their day-to-day work?

Yes, this patch fixed the problem. CI is back to >16 concurrency. This is locally verifiable by running tests and observing a superlinear in the number of cores slowndown prior to this patch in the runtime of temporal tests, and then running tests again with this patch and observing a totally linear test runtime regardless of the number of cores. I suggest reading through the incident channel - there's a lot of diagnosis and step-by-step information about what steps were taken and what impacts they had.

It is possible that the concurrent shutdown is the true fix, but that would be weird - we are observing an infinite freeze somewhere, and issuing the order to shutdown concurrently vs serially should not impact a process that is frozen.

If we're talking about footguns then wouldn't you say using an IORef is an even bigger one due to race conditions?

I believe this is addressed in the first part of the sentence you're responding to. Do you mind elaborating?

• assuming your workload is mostly updates at keys, not deletions or insertions.

That is not true. I think you are confused about how stm-containers works. A modification to one key or value of the map does not cause contention to any other key or value in the map.

Often a TVar containing a map will be just as good and much simpler since you're probably already using vanilla Maps anyway

"Simple" is a subjective judgment, but I'd be hard pressed to view significant complexity difference between

mv <- atomically $ StmMap.lookup k m
mv <- Map.lookup k <$> atomically (readTVar m)

and, even if i were to decide that StmMap.lookup k m is somehow more complex, it is undoubtedly warranted - diagnosing and fixing livelock issues like this takes an enormous amount of time, on top of the time wasted experiencing this bug. A patch to switch from TVar (Map k v) to stm-containers takes minutes to write, strictly makes things better, and avoids an entire class of performance problems.

A modification to one key or value of the map does not cause contention to any other key or value in the map

Yes that's what I said, but insertions and deletions still cause contention, even against updates at keys. Which is why it's so surprising to me that this patch would have had such an impact

Tbh I think the concurrent cancel might be the thing that did it: one workflow thread getting hung up (probably due to the safe-exceptions uninterruptible masking mistake) and causing the entire thing to block in excess of the configured timeout

A modification to one key or value of the map does not cause contention to any other key or value in the map

Yes that's what I said, but insertions and deletions still cause contention, even against updates at keys. Which is why it's so surprising to me that this patch would have had such an impact

No, it doesn't. You evidently don't understand the guarantees of stm-containers, which makes your position here much more understandable.

Tbh I think the concurrent cancel might be the thing that did it: one workflow thread getting hung up (probably due to the safe-exceptions uninterruptible masking mistake) and causing the entire thing to block in excess of the configured timeout

mapConcurrently_ cancel and mapM_ cancel will most likely have the same behavior, because mapConcurrently_ waits for all actions to complete before returning. For mapConcurrently_ alone to fix this, a running temporal workflow would have to block the shutdown of another unrelated temporal workflow, but in a manner where if they were both signaled to shut down, they would be able to shutdown cleanly together. And this would have to also result in livelock symptoms - ie more concurrency = more slowdown.

You're welcome to run the experiment if you want to - please post results when you do.

mapConcurrently_ cancel and mapM_ cancel will most likely have the same behavior, because mapConcurrently_ waits for all actions to complete before returning.

If one thread is unkillable then map_ cancel will stop at it, and those behind it in the list may never be killed because the thread that's killing the other threads is killed. That might explain the use of old database connections that we saw

But with mapConcurrently_ cancel, every other thread will more likely be killed by the time the killing thread is timed out, and only the unkillable one will remain

You evidently don't understand the guarantees of stm-containers, which makes your position here much more understandable.

I'm just looking at the source code for it and coming to my own conclusions. AFAICT insertions and deletions do contend with updates. Every focus reads the top-level TVar Hamt, even for an update, but insertions and removals also write it. Where am I mistaken?

I'm just looking at the source code for it and coming to my own conclusions. AFAICT insertions and deletions do contend with updates. Every focus reads the top-level TVar Hamt, even for an update, but insertions and removals also write it. Where am I mistaken?

I think I see it now: the focus data type finds the smallest branch to modify, therefore insertion/delete only contends with changes to nearby keys

If one thread is unkillable then map_ cancel will stop at it, and those behind it in the list may never be killed because the thread that's killing the other threads is killed. That might explain the use of old database connections that we saw

But with mapConcurrently_ cancel, every other thread will more likely be killed by the time the killing thread is timed out, and only the unkillable one will remain

Yes - this is what this other PR accomplished. After that PR, and prior to this one, we were no longer observing use of old database connections, but we were observing the freeze and the dramatic degradation in performance with increased concurrency. In this PR, we no longer observe the freeze or degradation in performance with increased concurrency.

While it is possible that uninterruptibleMask is the problem, the Temporal library also has extensive FFI use. Once control is passed from the GHC RTS to an FFI call, the thread is now uninterruptible until the call returns. You can have interruptible foreign calls with InterruptibleFFI, but this is not done in this library (nor is it clear that we should do that by default). However, if either of those were the problem, then it would be very strange indeed that stm-containers and/or concurrent shutdown would fix it, and it does not explain the slowdown-with-increased-concurrency.

IIUC that the test suite is like having 32+ mwb instances running on the same temporal shared state then I could see stm-containers being a factor: if there's one AcitivtyWorker/runningActivities map for the entire test suite--shared by all test cases and all test Apps--then that might be contended enough to make a noticeable difference. Still, that much contention seems like an issue itself, but could be one that we don't see in production