fix(transactions): return NotFoundError or (new) TypeMismatchError instead of StorageTransactionInconsistent where appropriate #1472

seefeldb · 2025-07-22T19:19:19Z

Improve storage transaction error handling with distinct error types

This PR enhances the storage transaction system by introducing a new TypeMismatchError to distinguish between different failure modes when accessing document properties. Previously, all such errors were reported as StorageTransactionInconsistent, which made it difficult to determine whether a retry would help.

Problem

The storage transaction system previously conflated two distinct types of errors:

Type mismatches - Trying to access properties on non-objects (e.g., "string".property)
Actual inconsistencies - When underlying data changed during the transaction

Both were reported as StorageTransactionInconsistent, but they have very different implications:

Type mismatches are persistent errors that will always fail, regardless of retries
True inconsistencies indicate the underlying data changed and a retry might succeed

Solution

This PR introduces a new TypeMismatchError to clearly distinguish these cases:

// Before: Both scenarios threw StorageTransactionInconsistent
doc.value = "not an object";
doc.value.property; // StorageTransactionInconsistent

// After: Clear distinction
doc.value = "not an object";  
doc.value.property; // TypeMismatchError - will always fail

Key Changes

New Error Type: Added ITypeMismatchError to the storage interface
Updated Error Handling: Modified attestation, chronicle, and transaction modules to use the appropriate error type
Backwards Compatibility: Transaction shim treats TypeMismatchError like NotFoundError for compatibility
Bug Fix: Chronicle now properly checks novelty before determining document existence
Documentation: Added comprehensive documentation explaining the transaction system and error handling

Error Types After This PR

StorageTransactionInconsistent - Read invariant violated (underlying state changed) - retry recommended
NotFoundError - Document or path doesn't exist - transient error
TypeMismatchError - Accessing properties on non-objects - persistent error
ReadOnlyAddressError - Writing to data: URIs - persistent error

Testing

Updated all existing tests to expect the correct error types
Added new comprehensive test suite for NotFound error scenarios
All tests pass with the new error handling

Documentation

Added two new documentation files:

transaction-explainer.md - High-level overview for users
transaction-implementation-guide.md - Detailed guide for maintainers

This change makes error handling more predictable and helps consumers make better decisions about whether to retry failed operations.

Summary by cubic

Improved storage transaction error handling by introducing a new TypeMismatchError and ensuring NotFoundError is returned when accessing non-existent documents or paths, instead of StorageTransactionInconsistent. This makes error types more precise and helps consumers decide when to retry operations.

Bug Fixes
- TypeMismatchError is now returned when accessing properties on non-object values.
- NotFoundError is returned for missing documents or paths.
- StorageTransactionInconsistent is only used for true state changes that may succeed on retry.
- Updated transaction-shim and all related modules and tests to use the correct error types.
Documentation
- Added detailed guides explaining the transaction system and new error handling.

- Add transaction-explainer.md: High-level overview for newcomers - Explains core architecture (StorageTransaction, Journal, Chronicle) - Covers key design principles (write isolation, read consistency) - Documents transaction lifecycle and usage patterns - Add transaction-implementation-guide.md: Detailed guide for maintainers - Documents state machines and transitions - Explains critical implementation patterns - Details error states and handling - Covers key algorithms (consistency checking, write merging) - Includes performance considerations and testing guidelines 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…types Align transaction implementation with transaction-shim.ts behavior by returning appropriate errors for different failure scenarios. Key change: StorageTransactionInconsistent errors now only occur when the underlying state has changed, maintaining the invariant that retrying after this error will see different data. This is the key indicator for when to retry a transaction. - Introduce TypeMismatchError for accessing properties on non-objects (e.g., reading string.property or null.field) - Return NotFoundError when documents or paths don't exist, instead of inconsistency errors - Fix bug where writes to nested paths of documents created in the same transaction would incorrectly fail with NotFoundError - Update all tests to expect appropriate error types - Update documentation to explain error type distinctions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…cefully The transaction shim's readOrThrow and writeOrThrow methods now treat TypeMismatchError like NotFoundError, returning undefined instead of throwing. This maintains backwards compatibility while leveraging the new distinct error types. Key changes: - validateParentPath now returns TypeMismatchError for type mismatches - readOrThrow returns undefined for both NotFoundError and TypeMismatchError - writeOrThrow handles both error types when creating parent entries - Updated tests to expect the correct error types 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

linear · 2025-07-22T19:19:21Z

CT-636 TX should return NotFound errors when accessing non-existent paths

seefeldb · 2025-07-22T19:20:14Z

Now we have 806 tests passing, 113 failing!

Copilot

Pull Request Overview

This PR enhances the storage transaction system by introducing a new TypeMismatchError to distinguish between type mismatches (accessing properties on non-objects) and actual data inconsistencies. Previously, both scenarios were reported as StorageTransactionInconsistent, making it difficult to determine whether retries would help.

Introduces ITypeMismatchError interface and implementation for persistent type errors
Updates error handling across transaction modules to use appropriate error types
Adds comprehensive documentation explaining the transaction system architecture

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/runner/test/transaction.test.ts	Updates test expectations from StorageTransactionInconsistent to TypeMismatchError for type mismatches
packages/runner/test/transaction-notfound.test.ts	New comprehensive test suite covering NotFound error scenarios and type mismatches
packages/runner/test/storage-transaction-shim.test.ts	Updates error expectations and messages for improved error distinction
packages/runner/test/journal.test.ts	Updates test expectations for type mismatch scenarios
packages/runner/test/chronicle.test.ts	Updates test expectations and adds proper type casting for ReadOnlyAddressError
packages/runner/test/attestation.test.ts	Updates error handling tests to expect TypeMismatchError and revised error messages
packages/runner/src/storage/transaction/chronicle.ts	Adds NotFoundError handling and converts errors during commit for consistency
packages/runner/src/storage/transaction/attestation.ts	Implements new TypeMismatchError class and updates error handling logic
packages/runner/src/storage/transaction-shim.ts	Enhances path validation to distinguish between NotFound and TypeMismatch errors
packages/runner/src/storage/transaction-implementation-guide.md	New detailed implementation guide for developers working on the transaction system
packages/runner/src/storage/transaction-explainer.md	New user-facing documentation explaining the transaction system architecture
packages/runner/src/storage/interface.ts	Adds ITypeMismatchError interface and updates error type unions

Comments suppressed due to low confidence (2)

packages/runner/test/transaction-notfound.test.ts:12

[nitpick] The class name 'MockReplica' is generic. Consider a more descriptive name like 'TestSpaceReplica' or 'InMemoryTestReplica' to better indicate its purpose as a test fixture.

class MockReplica implements ISpaceReplica {

packages/runner/test/transaction-notfound.test.ts:38

[nitpick] The class name 'MockStorageManager' is generic. Consider a more descriptive name like 'TestStorageManager' or 'InMemoryStorageManager' to better indicate its test-specific implementation.

class MockStorageManager implements IStorageManager {

packages/runner/src/storage/transaction/chronicle.ts

packages/runner/src/storage/transaction-shim.ts

cubic-dev-ai

cubic analysis

2 issues found across 12 files • Review in cubic

_{React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.}

cubic-dev-ai · 2025-07-22T19:24:00Z

packages/runner/src/storage/transaction-shim.ts

-    if (writeResult.error && writeResult.error.name === "NotFoundError") {
+    if (writeResult.error && 
+        (writeResult.error.name === "NotFoundError" || 
+         writeResult.error.name === "TypeMismatchError")) {


writeOrThrow incorrectly handles TypeMismatchError as if it were NotFoundError, which can overwrite existing non-object values and mask real errors

Prompt for AI agents

Address the following comment on packages/runner/src/storage/transaction-shim.ts at line 687: <comment>writeOrThrow incorrectly handles TypeMismatchError as if it were NotFoundError, which can overwrite existing non-object values and mask real errors</comment> <file context> @@ -644,9 +682,13 @@ export class ExtendedStorageTransaction implements IExtendedStorageTransaction { value: JSONValue | undefined, ): void { const writeResult = this.tx.write(address, value); - if (writeResult.error && writeResult.error.name === "NotFoundError") { + if (writeResult.error && + (writeResult.error.name === "NotFoundError" || + writeResult.error.name === "TypeMismatchError")) { // Create parent entries if needed - const lastValidPath = writeResult.error.path; </file context>

cubic-dev-ai · 2025-07-22T19:24:00Z

packages/runner/src/storage/transaction-shim.ts

-    if (readResult.error && readResult.error.name !== "NotFoundError") {
+    if (readResult.error && 
+        readResult.error.name !== "NotFoundError" && 
+        readResult.error.name !== "TypeMismatchError") {


readOrThrow treats TypeMismatchError as non-fatal, swallowing a persistent error that should be thrown

Prompt for AI agents

Address the following comment on packages/runner/src/storage/transaction-shim.ts at line 653: <comment>readOrThrow treats TypeMismatchError as non-fatal, swallowing a persistent error that should be thrown</comment> <file context> @@ -612,7 +648,9 @@ export class ExtendedStorageTransaction implements IExtendedStorageTransaction { options?: IReadOptions, ): JSONValue | undefined { const readResult = this.tx.read(address, options); - if (readResult.error && readResult.error.name !== "NotFoundError") { + if (readResult.error && + readResult.error.name !== "NotFoundError" && + readResult.error.name !== "TypeMismatchError") { throw readResult.error; } </file context>

…hError handling - Update storage.test.ts to work with new transaction system (shim=false) - Remove sync() calls and add explicit commits before querying - Add conditional sync() calls only when shim is true - Wait for storage processing with runtime.idle() after commits - Fix data structure expectations to match new format - Update storage-transaction-shim.test.ts expectations - Change line 309 to expect TypeMismatchError instead of NotFoundError - This aligns with the shim behavior that distinguishes type mismatches - Previously updated transaction-shim.ts to treat TypeMismatchError like NotFoundError - Both error types are now handled gracefully in readOrThrow and writeOrThrow 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…ions Updated test expectations to match the new subscription behavior where 'before' and 'after' fields now always contain the full document rather than just the value at the specified path. Changes: - Fixed reactive-dependencies tests to pass full document structure - Updated storage-subscription tests to expect full document with wrapper - Modified storage-transaction-shim tests for consistent document structure - Adjusted expectations for empty documents to use {} instead of undefined 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

seefeldb · 2025-07-22T21:51:58Z

USE_REAL_TRANSACTIONS=1 deno task test is now down to 70 failing tests. @ubik2 PTAL and I think I'll merge it in this state, since there were a bunch of fixes that aligned the shim behavior to the real thing as well.

cubic-dev-ai

cubic analysis

2 issues found across 19 files • Review in cubic

_{React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.}

packages/runner/src/runtime.ts

packages/runner/src/cell.ts

packages/runner/src/storage/transaction-shim.ts

ubik2 · 2025-07-22T22:48:15Z

packages/runner/src/storage/interface.ts

 export interface INotFoundError extends Error {
  name: "NotFoundError";
  path?: MemoryAddressPathComponent[];
+  from(space: MemorySpace): INotFoundError;


It looks like we lost the source: IAttestation and address: IMemoryAddress fields here.
It's not clear if that was intentional or accidental, since the interface was double defined with half here, and half later.

Good catch, and the implementation in NotFound actually sets those. Added.

ubik2 · 2025-07-22T22:57:13Z

packages/runner/src/storage/transaction/attestation.ts


        return { ok: patch };
      } else {
+        // Type mismatch - trying to write property on non-object


This is somewhat outside the PR, but on line 82 of the new file, we cast what can probably be an array into a Record<string, JSONValue>.
This works here, where we don't know the type, but wouldn't be a valid cast if typescript knew it was an array (you'd have to do an as unknown).
The behavior is correct, and maybe it's intentional to avoid code duplication just to appease types.

good catch. i added tighter checking for arrays as well now (key is either a >= 0 integer or 'length')

ubik2

lgtm
some minor comments, but looks good to merge as is.

Update attestation.write() to properly handle array types: - Explicitly check for arrays using Array.isArray() - Only allow writing to arrays at: - Valid non-negative integer indices (0, 1, 2, etc.) - The 'length' property for array truncation - Return TypeMismatchError for invalid array operations Add comprehensive tests to validate the new behavior: - Writing to valid positive integer indices (0, 1, 10) - Writing to array 'length' property for truncation - TypeMismatchError for negative indices (-1) - TypeMismatchError for non-integer keys (1.5) - TypeMismatchError for string keys other than 'length' - Proper handling of sparse arrays This ensures arrays are treated distinctly from objects and prevents invalid array modifications while maintaining backwards compatibility. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

seefeldb and others added 3 commits July 22, 2025 12:12

seefeldb requested review from Gozala, Copilot and ubik2 July 22, 2025 19:19

Copilot AI reviewed Jul 22, 2025

View reviewed changes

seefeldb marked this pull request as draft July 22, 2025 19:20

seefeldb temporarily deployed to ci July 22, 2025 19:21 — with GitHub Actions Inactive

cubic-dev-ai bot reviewed Jul 22, 2025

View reviewed changes

seefeldb temporarily deployed to ci July 22, 2025 19:24 — with GitHub Actions Inactive

seefeldb and others added 2 commits July 22, 2025 16:03

control whether to use new transactions via env variable

5a5c596

seefeldb temporarily deployed to ci July 22, 2025 21:08 — with GitHub Actions Inactive

seefeldb temporarily deployed to ci July 22, 2025 21:10 — with GitHub Actions Inactive

seefeldb temporarily deployed to ci July 22, 2025 21:51 — with GitHub Actions Inactive

seefeldb marked this pull request as ready for review July 22, 2025 21:51

seefeldb temporarily deployed to ci July 22, 2025 21:52 — with GitHub Actions Inactive

cubic-dev-ai bot reviewed Jul 22, 2025

View reviewed changes

packages/runner/src/runtime.ts Outdated Show resolved Hide resolved

packages/runner/src/cell.ts Outdated Show resolved Hide resolved

ubik2 reviewed Jul 22, 2025

View reviewed changes

packages/runner/src/storage/transaction-shim.ts Outdated Show resolved Hide resolved

ubik2 reviewed Jul 22, 2025

View reviewed changes

ubik2 approved these changes Jul 22, 2025

View reviewed changes

address PR feedback

9835f37

seefeldb temporarily deployed to ci July 23, 2025 14:37 — with GitHub Actions Inactive

seefeldb temporarily deployed to ci July 23, 2025 14:39 — with GitHub Actions Inactive

seefeldb and others added 2 commits July 23, 2025 09:45

re-add fields to INotFoundError

48b2b2b

seefeldb temporarily deployed to ci July 23, 2025 14:52 — with GitHub Actions Inactive

seefeldb temporarily deployed to ci July 23, 2025 14:54 — with GitHub Actions Inactive

fix comment to reflect new names

377b6ce

seefeldb temporarily deployed to ci July 23, 2025 15:01 — with GitHub Actions Inactive

seefeldb temporarily deployed to ci July 23, 2025 15:02 — with GitHub Actions Inactive

seefeldb merged commit d6b0480 into main Jul 23, 2025
7 checks passed

seefeldb deleted the berni/ct-636-tx-should-return-notfound-errors-when-accessing-non-existent branch July 23, 2025 15:33

fix(transactions): return NotFoundError or (new) TypeMismatchError instead of StorageTransactionInconsistent where appropriate #1472

fix(transactions): return NotFoundError or (new) TypeMismatchError instead of StorageTransactionInconsistent where appropriate #1472

Uh oh!

Conversation

seefeldb commented Jul 22, 2025 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Improve storage transaction error handling with distinct error types

Problem

Solution

Key Changes

Error Types After This PR

Testing

Documentation

Summary by cubic

Uh oh!

linear bot commented Jul 22, 2025

Uh oh!

seefeldb commented Jul 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

cubic analysis

Uh oh!

cubic-dev-ai bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

seefeldb commented Jul 22, 2025

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

cubic analysis

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ubik2 Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

seefeldb Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

ubik2 Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

seefeldb Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

ubik2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seefeldb commented Jul 22, 2025 •

edited by cubic-dev-ai bot

Loading