Skip to content

fix(ops): make add_entity atomic — snapshot + rollback on validation gate failure #91

Open

Description

@coderabbitai

Summary

GtsOps::add_entity (and transitively add_entities) is non-atomic: self.store.register(entity.clone()) runs first, and every subsequent validation gate (validate_schema_modifiers, validate_trait_placement, validate_schema, validate_schema_chain, validate_schema_traits) returns ok: false on failure without rolling back the registration. Rejected entities therefore remain in the store and are visible to later reads and validations.

This pattern predates PR #90 (v0.12 support) and is systemic — it was not introduced by the trait-placement check added in that PR.

Desired behaviour

  • Snapshot + rollback: before running any validation gate, snapshot (or skip registering until all gates pass), and roll back / evict the entity from the store if any gate fails.
  • Rollback-safe re-registration: a failed re-registration of an already-registered entity must not wipe the previously-registered valid state.
  • Batch transactionality: add_entities should either commit all or none (or at minimum clearly document per-item rollback behaviour).

References

/cc @aviator5

Activity

  1. aviator5 commented on Jun 12, 2026

    @aviator5
    Member

    Related but distinct: validate_schema (/validate-type-schema) never runs the placement/modifier gates

    While the non-atomic add_entity is the systemic root cause described here, there's a separate consistency gap worth tracking alongside it: the three schema entry points are not aligned on which validation gates they run before trait merge.

    Endpoint ops method runs validate_trait_placement / validate_schema_modifiers?
    POST /entities add_entity (ops.rs:281) yes (ops.rs:332, :348)
    POST /validate-entity validate_entity (ops.rs:677) yes (ops.rs:682, :695)
    POST /validate-type-schema validate_schema (ops.rs:643) no — goes straight to validate_schema_traits (ops.rs:663)

    So validate_schema calls into the trait pipeline without ever checking keyword placement.

    How it composes with this issue. The two together produce a silent-wrong-result path:

    1. POST /entities with a derived schema that nests x-gts-traits inside an allOf entry → register() stores it (ops.rs:317), the placement gate fails (ops.rs:348) and returns 422 — but per this issue the rejected schema lingers in the store.
    2. POST /validate-type-schema for that same id → validate_schema (no placement gate) → collect_traits_from_value recurses into allOf (schema_traits.rs:406-410) and silently merges the misplaced x-gts-traits instead of rejecting it (contradicting §9.7.1 fail-fast).

    Implication for the fix here. Snapshot + rollback (the desired behaviour above) removes step 1's precondition, so the misplaced schema never persists and step 2 becomes practically unreachable. That closes the exploitable path. What remains afterwards is a softer, defense-in-depth alignment, which could be folded into this work or split off:

    • Run the same placement/modifier gates in validate_schema that add_entity/validate_entity already run, so all three entry points behave identically regardless of how a schema entered the store.
    • Optional cleanup: the allOf recursion in collect_trait_schema_from_value / collect_traits_from_value (schema_traits.rs:344-411) is vestigial post-v0.12 — under §9.7.1 these keywords are top-level-only, so the recursion can only ever pick up a spec-forbidden placement. Its comment ("e.g. a derived schema that is an allOf overlay") is now misleading.

    Happy to split the validate_schema alignment into its own issue if it's out of scope for the atomicity fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions