Skip to content

Handle OOM-style definition clone failures safely#70

Open
Lihi-AI wants to merge 2 commits into
mainfrom
fix/clone-create-oom-policy
Open

Handle OOM-style definition clone failures safely#70
Lihi-AI wants to merge 2 commits into
mainfrom
fix/clone-create-oom-policy

Conversation

@Lihi-AI

@Lihi-AI Lihi-AI commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR defines and applies a narrow nullable contract for definition clone/create paths under Loka's no-exception policy.

  • stage replacement clones before swapping owned WindowProps definitions
  • return 0 from WindowDefinition::create() when owned definition cloning fails
  • invalidate composition snapshots when cloning fails so stale diffs cannot be applied
  • transfer the already-cloned window root into Scene instead of cloning it twice
  • document that nullable clone() / create() results are reserved for allocation-style failure such as OOM

Why

clone() == 0 previously mixed together “empty”, contract misuse, and allocation failure depending on the callsite. This change narrows the contract:

  • contract misuse should be rejected structurally or by debug assert
  • allocation-style failure is the intended runtime nullable case
  • owner-side replacement code preserves the previous owned value when staging fails
  • cache-like composition snapshots invalidate themselves on failure instead of retaining stale data

Impact Scope

Affected:

  • WindowProps owned root/menu definition copy, assignment, and replacement
  • WindowDefinition::create() root-to-Scene ownership transfer and failure cleanup
  • NodeCompositionSnapshot clone-failure invalidation
  • documentation of clone/create nullable semantics

Not affected:

  • normal successful definition composition and state propagation
  • platform projection or drawing behavior
  • allocator-wide OOM recovery for every concrete clone/create implementation

Concrete implementations that still use plain new, or whose constructors allocate internally, remain explicit TODO migration targets. This PR defines the meaning and owner-side handling of nullable results; it does not claim end-to-end allocator recovery is complete.

Validation

  • cmake --build build/Testing
  • ctest --test-dir build/Testing --output-on-failure
  • cmake --build build/Testing-ASan
  • ctest --test-dir build/Testing-ASan --output-on-failure
  • Win32 HelloWorld manual runtime check

Red evidence added during review hardening:

  • snapshot test failed because a clone failure retained the previous root
  • window test failed because one root materialization called clone() twice

Both are green in Testing and Testing-ASan after 8e76976.

Closes #66.

@cubenoy22 cubenoy22 requested a review from Copilot July 8, 2026 00:59
@cubenoy22 cubenoy22 marked this pull request as ready for review July 8, 2026 00:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR clarifies and hardens Loka’s no-exception allocation-failure policy for definition cloning/materialization by treating 0 returns as OOM-style failures, staging replacement clones before swapping owned definitions, and adding regression tests to pin “stable on failure” behavior.

Changes:

  • Updated WindowProps assignment/setter paths to stage owned-definition clones and preserve the prior owned value if staging fails.
  • Hardened WindowDefinition::create() to return 0 when owned definition cloning fails, and documented the intended nullable contract for clone() / create().
  • Added focused regression tests plus documentation updates to codify the allocation-failure policy and migration targets.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/FlowDslTestMain.cpp Updates test runner to call the new/renamed clone-failure regression tests.
tests/DefinitionCloneTests.hpp Renames and expands clone-failure test declarations.
tests/DefinitionCloneTests.cpp Adds OOM-style probe definitions, a null platform context, and new regression tests for failure propagation/stability.
docs/TODO.md Notes remaining legacy clone seams that still blur “empty” vs allocation failure.
docs/ProgrammingGuide.en.md Documents the “keep allocation failure narrow” policy and intended semantics of nullable clone() / create().
common/app/scene/Node.hpp Adds in-code documentation clarifying the allocation/nullable contract for node definition materialization.
common/app/scene/composition/NodeCompositionSnapshot.hpp Refactors snapshot replacement to clone-then-swap behavior (used for diffing compositions).
common/app/core/WindowDefinition.hpp Returns 0 from create() when owned-definition cloning fails (pre-window creation).
common/app/core/Window.hpp Stages owned-definition clones in WindowProps copy/assignment and in scene()/menuBar() setters to preserve stable state on failure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread common/app/scene/composition/NodeCompositionSnapshot.hpp
Comment thread common/app/core/WindowDefinition.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define no-exception OOM policy for definition clone/create paths

3 participants