Turbopack: use the same serialization method for lookup as for storing (#84765)

### What?

This makes sure to use the same method for serializing CachedTaskType for storing and lookup.
Before that we instantiated the serialization twice and that caused some weird behavior on linux.

Before this fix I was seeing that serialization of CachedTaskType was not byte to byte identical between storing and lookup. This caused tasks to not be found, which will lead to very weird effects: stale tasks, cell not found, etc.

I was expecting that pot serialization is deterministic, but turns out it isn't fully deterministic. It has some logic to deduplicate Symbols (like enum variant names). But it deduplicates these `&'static str`s based on the pointer address of the static string. Turns out that Rust doesn't guarantee that static strings are deduplicated in all cases.

I assume that we are somehow running into that problem when having two variants of the serialization. They ended up serializing task types differently (one didn't have a symbol deduplicated in the serialized form. But that's more a wild guess. It did fix the bug, but I have a hard time understanding why this is happening.

On long term, I think we should change pot to avoid using pointer addresses and do a real comparision instead.

> why does the `inline(never)` fix it?

I'm not super sure, but here is my theory. The rust compiler on linux is very aggressive in inlining the whole stuff, up to the point where it inlines the Serialize implementation of CachedTaskType and RawVc into the call side (`turbo-tasks-backend` crate). It can't inline the Serialize implementation that's behind the `arg` `MagicAny` stuff. So we are duplicating the RawVc Serialize logic into two crates. This way we end up with two `&'static str` of `TaskCell` for the enum variant. This string is not deduplicated during linking for whatever reason (It's not guaranteed). So in the end the Symbol is not deduplicated in the serialized form of the CachedTaskType.

The stuff above might happen or not. It's in the fate of the compiler. And we also don't rely on that the Symbol is deduplicated. But we rely on a deterministic serialized version. The problem is that the lookup path and the store path have two separate calls to `serialize`, so it might end up inlining into two different places. So that might end up being aggressive in one place and not so aggressive in the other place. So in the end it might end up deduplicating in the lookup path and not deduplicating in the store path, which would cause different serialized versions in the two paths.

To avoid that, I changed to code to use the `serialize_task_type` function in both places. And the `inline(never)` ensures that it won't be duplicated by inlining.

Don't ask me how long it took to figure that out...

Author: sokra

turbopack/crates/turbo-tasks-backend/src/kv_backing_storage.rs

@@ -357,8 +357,12 @@ impl<T: KeyValueDatabase + Send + Sync + 'static> BackingStorageSealed
 
                             let mut task_type_bytes = Vec::new();
                             for (task_type, task_id) in updates {
+                                serialize_task_type(
+                                    &task_type,
+                                    &mut task_type_bytes,
+                                    Some(task_id),
+                                )?;
                                 let task_id: u32 = *task_id;
-                                serialize_task_type(&task_type, &mut task_type_bytes, task_id)?;
 
                                 batch
                                     .put(
@@ -441,8 +445,8 @@ impl<T: KeyValueDatabase + Send + Sync + 'static> BackingStorageSealed
                     .entered();
                     let mut task_type_bytes = Vec::new();
                     for (task_type, task_id) in task_cache_updates.into_iter().flatten() {
+                        serialize_task_type(&task_type, &mut task_type_bytes, Some(task_id))?;
                         let task_id = *task_id;
-                        serialize_task_type(&task_type, &mut task_type_bytes, task_id)?;
 
                         batch
                             .put(
@@ -499,8 +503,10 @@ impl<T: KeyValueDatabase + Send + Sync + 'static> BackingStorageSealed
             tx: &D::ReadTransaction<'_>,
             task_type: &CachedTaskType,
         ) -> Result<Option<TaskId>> {
-            let task_type = POT_CONFIG.serialize(task_type)?;
-            let Some(bytes) = database.get(tx, KeySpace::ForwardTaskCache, &task_type)? else {
+            let mut task_type_bytes = Vec::new();
+            serialize_task_type(task_type, &mut task_type_bytes, None)?;
+            let Some(bytes) = database.get(tx, KeySpace::ForwardTaskCache, &task_type_bytes)?
+            else {
                 return Ok(None);
             };
             let bytes = bytes.borrow().try_into()?;
@@ -645,23 +651,37 @@ where
     Ok(())
 }
 
+// DO NOT REMOVE THE `inline(never)` ATTRIBUTE!
+// `pot` uses the pointer address of `&'static str` to deduplicate Symbols.
+// If this function is inlined into multiple different callsites it might inline the Serialize
+// implementation too, which can pull a `&'static str` from another crate into this crate.
+// Since string deduplication between crates is not guaranteed, it can lead to behavior changes due
+// to the pointer addresses. This can lead to lookup path and store path creating different
+// serialization of the same task type, which breaks task cache lookups.
+#[inline(never)]
 fn serialize_task_type(
-    task_type: &Arc<CachedTaskType>,
+    task_type: &CachedTaskType,
     mut task_type_bytes: &mut Vec<u8>,
-    task_id: u32,
+    task_id: Option<TaskId>,
 ) -> Result<()> {
     task_type_bytes.clear();
     POT_CONFIG
-        .serialize_into(&**task_type, &mut task_type_bytes)
-        .with_context(|| anyhow!("Unable to serialize task {task_id} cache key {task_type:?}"))?;
+        .serialize_into(task_type, &mut task_type_bytes)
+        .with_context(|| {
+            if let Some(task_id) = task_id {
+                anyhow!("Unable to serialize task {task_id} cache key {task_type:?}")
+            } else {
+                anyhow!("Unable to serialize task cache key {task_type:?}")
+            }
+        })?;
     #[cfg(feature = "verify_serialization")]
     {
         let deserialize: Result<CachedTaskType, _> = serde_path_to_error::deserialize(
             &mut pot_de_symbol_list().deserializer_for_slice(&*task_type_bytes)?,
         );
         if let Err(err) = deserialize {
-            println!("Task type would not be deserializable {task_id}: {err:?}\n{task_type:#?}");
-            panic!("Task type would not be deserializable {task_id}: {err:?}");
+            println!("Task type would not be deserializable {task_id:?}: {err:?}\n{task_type:#?}");
+            panic!("Task type would not be deserializable {task_id:?}: {err:?}");
         }
     }
     Ok(())