Revert "Serialize workflow-version canonical bytes once at construction"

This reverts commit 8090d7030984862564a929ee9264e93911014e00.

The cached canonical field was optimizing an unmeasured path: without
the (deferred) O(closure) dependency re-validation multiplier, the
repeated serialization is microseconds for realistic versions. Compute
canonical bytes on demand like the environment, automation, and MCP
stores do, rather than carrying a serde-skipped cache field, a
construction bootstrap, and doubled memory for it. Purely in-memory:
stored blobs and version IDs are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-13 16:02:12 -04:00
parent 180330c117
commit 8c3ff6216c
2 changed files with 36 additions and 32 deletions

View file

@ -2,7 +2,7 @@ use std::collections::{BTreeMap, HashSet, VecDeque};
use std::sync::Arc;
use fabro_store::BlobStore;
use fabro_types::{WorkflowPath, WorkflowVersion, WorkflowVersionId};
use fabro_types::{WorkflowPath, WorkflowVersion, WorkflowVersionId, WorkflowVersionShapeError};
use thiserror::Error;
use crate::{ValidatedWorkflowVersion, WorkflowVersionError};
@ -11,6 +11,8 @@ use crate::{ValidatedWorkflowVersion, WorkflowVersionError};
pub enum WorkflowVersionStoreError {
#[error(transparent)]
InvalidVersion(#[from] WorkflowVersionError),
#[error(transparent)]
InvalidShape(#[from] WorkflowVersionShapeError),
#[error("workflow-version dependency `{id}` at `{path}` is not stored")]
DependencyNotFound {
path: WorkflowPath,
@ -58,10 +60,11 @@ impl WorkflowVersionStore {
&self,
version: &ValidatedWorkflowVersion,
) -> Result<WorkflowVersionId, WorkflowVersionStoreError> {
let canonical = version.version().canonical_bytes()?;
self.validate_dependency_closure(version.version().workflow_dependencies())
.await?;
self.blobs
.write(version.version().canonical_bytes())
.write(&canonical)
.await
.map(WorkflowVersionId::from)
.map_err(|source| WorkflowVersionStoreError::Storage { source })
@ -95,7 +98,8 @@ impl WorkflowVersionStore {
let version = serde_json::from_slice::<WorkflowVersion>(&bytes)
.map_err(|source| WorkflowVersionStoreError::Decode { id: *id, source })?;
let validated = ValidatedWorkflowVersion::new(version)?;
if validated.version().canonical_bytes() != bytes.as_ref() {
let canonical = validated.version().canonical_bytes()?;
if canonical.as_slice() != bytes.as_ref() {
return Err(WorkflowVersionStoreError::NonCanonical { id: *id });
}
Ok(Some(validated))
@ -192,7 +196,7 @@ mod tests {
async fn put_get_reuses_exact_blob_digest() {
let (blobs, store) = stores().await;
let version = version("digraph W {}", BTreeMap::new());
let expected_bytes = version.version().canonical_bytes().to_vec();
let expected_bytes = version.version().canonical_bytes().unwrap();
let expected_id = WorkflowVersionId::from(fabro_types::BlobHash::new(&expected_bytes));
let id = store.put(&version).await.unwrap();
@ -224,14 +228,15 @@ mod tests {
let (blobs, store) = stores().await;
let child = version("digraph Child {}", BTreeMap::new());
let child_id = WorkflowVersionId::from(fabro_types::BlobHash::new(
child.version().canonical_bytes(),
&child.version().canonical_bytes().unwrap(),
));
let root = version(
r#"digraph Root { child [stack.child_workflow="child.fabro"] }"#,
BTreeMap::from([(path("child.fabro"), child_id)]),
);
let root_id =
WorkflowVersionId::from(fabro_types::BlobHash::new(root.version().canonical_bytes()));
let root_id = WorkflowVersionId::from(fabro_types::BlobHash::new(
&root.version().canonical_bytes().unwrap(),
));
let error = store.put(&root).await.unwrap_err();
assert!(matches!(
@ -251,14 +256,15 @@ mod tests {
r#"digraph Child { grandchild [stack.child_workflow="grandchild.fabro"] }"#,
BTreeMap::from([(path("grandchild.fabro"), missing_grandchild_id)]),
);
let child_bytes = child.version().canonical_bytes();
let child_id = WorkflowVersionId::from(blobs.write(child_bytes).await.unwrap());
let child_bytes = child.version().canonical_bytes().unwrap();
let child_id = WorkflowVersionId::from(blobs.write(&child_bytes).await.unwrap());
let root = version(
r#"digraph Root { child [stack.child_workflow="child.fabro"] }"#,
BTreeMap::from([(path("child.fabro"), child_id)]),
);
let root_id =
WorkflowVersionId::from(fabro_types::BlobHash::new(root.version().canonical_bytes()));
let root_id = WorkflowVersionId::from(fabro_types::BlobHash::new(
&root.version().canonical_bytes().unwrap(),
));
assert!(matches!(
store.put(&root).await.unwrap_err(),

View file

@ -53,8 +53,6 @@ pub struct WorkflowVersion {
entrypoint: WorkflowPath,
files: BTreeMap<WorkflowPath, String>,
workflow_dependencies: BTreeMap<WorkflowPath, WorkflowVersionId>,
#[serde(skip)]
canonical: Vec<u8>,
}
impl WorkflowVersion {
@ -63,22 +61,13 @@ impl WorkflowVersion {
files: BTreeMap<WorkflowPath, String>,
workflow_dependencies: BTreeMap<WorkflowPath, WorkflowVersionId>,
) -> Result<Self, WorkflowVersionShapeError> {
let mut version = Self {
let version = Self {
entrypoint,
files,
workflow_dependencies,
canonical: Vec::new(),
};
version.validate_shape()?;
let canonical = serde_json::to_vec(&version)
.map_err(|source| WorkflowVersionShapeError::Serialization { source })?;
if canonical.len() > MAX_WORKFLOW_VERSION_BYTES {
return Err(WorkflowVersionShapeError::VersionTooLarge {
actual: canonical.len(),
maximum: MAX_WORKFLOW_VERSION_BYTES,
});
}
version.canonical = canonical;
version.canonical_bytes()?;
Ok(version)
}
@ -97,11 +86,20 @@ impl WorkflowVersion {
&self.workflow_dependencies
}
/// Canonical wire bytes, serialized and size-checked once at
/// construction.
#[must_use]
pub fn canonical_bytes(&self) -> &[u8] {
&self.canonical
/// Serialize to the canonical wire form.
///
/// Structural validity is guaranteed by construction, so this only
/// serializes and enforces the canonical size limit.
pub fn canonical_bytes(&self) -> Result<Vec<u8>, WorkflowVersionShapeError> {
let bytes = serde_json::to_vec(self)
.map_err(|source| WorkflowVersionShapeError::Serialization { source })?;
if bytes.len() > MAX_WORKFLOW_VERSION_BYTES {
return Err(WorkflowVersionShapeError::VersionTooLarge {
actual: bytes.len(),
maximum: MAX_WORKFLOW_VERSION_BYTES,
});
}
Ok(bytes)
}
fn validate_shape(&self) -> Result<(), WorkflowVersionShapeError> {
@ -252,7 +250,7 @@ mod tests {
.unwrap();
assert_eq!(
String::from_utf8(version.canonical_bytes().to_vec()).unwrap(),
String::from_utf8(version.canonical_bytes().unwrap()).unwrap(),
r#"{"entrypoint":"workflow.fabro","files":{"a.txt":"A","workflow.fabro":"digraph W {}","z.txt":"Z"},"workflow_dependencies":{}}"#
);
}
@ -421,7 +419,7 @@ mod tests {
BTreeMap::new(),
)
.unwrap();
let remaining = MAX_WORKFLOW_VERSION_BYTES - empty.canonical_bytes().len();
let remaining = MAX_WORKFLOW_VERSION_BYTES - empty.canonical_bytes().unwrap().len();
let per_file = remaining / 4;
let remainder = remaining % 4;
for index in 0..4 {
@ -436,7 +434,7 @@ mod tests {
)
.unwrap();
assert_eq!(
exact_version.canonical_bytes().len(),
exact_version.canonical_bytes().unwrap().len(),
MAX_WORKFLOW_VERSION_BYTES
);
exact_version_files