Repository navigation
fix(reliability): make event matching atomic and close native lifecycle gaps - #260
Merged
Merged
Conversation
ovasylenko
marked this pull request as ready for review
October 23, 2026 05:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Orch8 reliability review — 23 October 2026
Review complete. All suitable fixes are merged to main
77c60ae; all 22 main CI/Mobile jobs passed.Scope: all 13 Cargo workspace crates, the standalone Node/Python native bindings, SDK contract checks, CI, and open pull requests. The review concentrated on durable state transitions, cancellation/restart, original execution claims, tenant boundaries, resource ownership, and failure recovery. It is a boundary-focused review, not a claim that every module or platform has exhaustive coverage.
Changes and evidence
ClaimedControl; PostgreSQL row locks and SQLite immediate transactions validate ownership with writes. Includes loops, races, cancellation, Saga, child creation/adoption and SLA state. PRs #249, #251, #257, #258.--locked.These are eight remediation groups, not eight independently counted defects. Repeated symptoms of the same ownership/transaction boundary are grouped together.
Crate coverage
orch8orch8-typesorch8-storageorch8-engineorch8-mobileorch8-publisherorch8-pushorch8-apiorch8-grpcorch8-serverorch8-cliorch8-wasmorch8-e2eThe excluded Node and Python native manifests are checked separately with
--locked; their shared Rust implementation is exercised through ordinary facade integration tests. Platform SDK workflows remain a separate merge gate.Implementation anchors
The follow-up storage contract is in
orch8-storage/src/lib.rs:1659(register_event_wait) and:1677(match_event_wait); both nativeevents.rsimplementations commit the three facts in one database transaction. The engine paths use the primitive inorch8-engine/src/event_correlation.rs. Original ownership is represented byorch8-engine/src/control.rs:27(ClaimedControl). Explicit owned-pool shutdown isorch8/src/engine.rs:196; the shared Node/Python lifecycle ispackages/native-common/durable.rs.Regression entry points:
orch8-storage/tests/event_match_transactions.rs,orch8-storage/tests/claimed_gate_registration.rs,orch8-engine/tests/event_atomic_recovery.rs,event_wait_iteration.rs,event_payload_reads.rs,prepark_claim_authority.rs, andorch8/tests/native_initialization_retry.rs. These exercise observable state and runtime effects rather than duplicating implementation branches.Technical opportunity synthesis
The highest-impact structural correction is to combine immutable ownership capabilities, native database transactions and the existing fault-injecting backend matrix. This gives composite control, event matching, restoration and lifecycle code explicit commit boundaries that can be tested with real failures. It removes whole classes of check-then-write and partial-commit failures without a new framework or service split.
The concrete baseline was reproducible corruption, lost progress or retained resources. The acceptance criterion is that the same probes preserve the replacement owner's state, leave a recoverable pending fact on rollback, commit a unique notification, or release owned resources. No production incident rate, latency multiplier or financial ROI is asserted; those measurements were not available.
Keep independent federation/outbox leases and terminal compensation separate from scheduler-owned claims. Preserve append-only real step outputs, documented explicit federation
call_keyidentity, existing terminal-wait behavior, and acknowledgement semantics. External side effects still require their documented idempotency/retry contracts.Pull-request dispositions
The original CI request was the Criterion nested-Tokio-runtime panic in
evaluate_flat_10_steps(failed job). PR #225 merged the benchmark correction as769b2e4a; the replacement Benchmarks run completed successfully. This verifies benchmark execution, not a performance improvement.The recent reviewed and merged history additionally includes #228 and #231–#259 (30 PRs). Before #260, runtime PR #259 was merged only after all 22 checks and both CI/Mobile workflows succeeded on exact head
2eb960804605f393ca777a1b33e645d708aa0cd7. Main363b6e0ac9c09b6be695ca85ec16b7d63cc11ac5subsequently passed CI39888731788and Mobile39888731789.Not suitable as submitted, left open deliberately:
PR #260 merged as main
32569e9a46712a7596184b72d7d3754556db7455at 06:53:22 UTC. Reviewed head4123274a396f1713cfea800b92b6a76028fe37bb(local27b360e2) and merged main have identical source tree038cc0e275e52dffe922936b882cc4bb662e938e; the source diff is empty. Previous head76451ca9passed 21 checks, including Mobile, E2E, Docker, fuzz, lint and smoke checks. Its full CI unit job exposed two PostgreSQL fixture failures: unchanged persistedEventWaitrows had microsecond timestamps while the expected input retained nanoseconds. Explicit nanosecond input reproduced both failures locally. The correction compares the complete persisted row before/after reset, preserving all ownership and 405-deletion assertions; matching fixtures also force nanoseconds independently of the host clock. All 48 native/encrypted PostgreSQL/SQLite cases and strict workspace Clippy pass. Only those two test files changed; production code is unchanged from76451ca9.All 22 checks completed successfully before merge on exact reviewed head
4123274a; both complete CI and Mobile workflows succeeded. The merge used--match-head-commitafter rechecking head/base, feedback and the complete matrix. New main CI remains in progress; the complete Mobile workflow passed on exact main32569e9a. API-contract publication succeeded. The live OpenAPI serves HTTP 200 and its canonical fingerprint matches the revieweddc801bbd…exactly; the live authoring schema also serves correctly withnameandblocksrequired.SDK repository audit
All 13 SDK/contract/mobile-example repositories were checked for open PRs and remote branch heads. Each has only
main; none has an open PR. Existing local SDK checkouts match their remote main heads. The remaining local Nodefeat/workflow-builder-docsbranch is historical: its first two commits are the exact head of merged sdk-node#1, and its worker-protocol commit is patch-equivalent to merged sdk-node#2. No new source work was found there.sdk-node(Node 18/20/22),sdk-go(Go 1.21/1.22/1.23),sdk-python(Python 3.10–3.13),sdk-expo(package plus iOS/Android example builds),sdk-contract(Node 20/22 conformance kit),sdk-php(PHP 8.2–8.4),sdk-ruby(Ruby 3.1–3.4),sdk-dotnet(.NET 8 on Linux/Windows),sdk-java(Java 17/21)react-native-orch8,orch8-flutter,orch8-mobile-swift,mobile-examplesThe absent check records are reported explicitly; the engine's separate current-head Mobile workflow supplies additional platform evidence, not an exact-tree test claim for those four repositories.
Verification record
Final main
77c60aeb9d4aeabd0ace5a66348831c68bb65b40has the exact reviewed source treeae988ae56d6d1896add23421b863f02781b42506; its source diff against reviewed head89747e94is empty. All ten CI jobs and all twelve Mobile jobs completed successfully. The final Linux log confirms 9,910 passed, zero failed, ten existing ignored cases across 238 groups, plus 40 successful Criterion smoke cases. Strict workspace/all-target Clippy, warning-free docs, standalone locked Node/Python checks, generated Rust/JavaScript client compile-and-execute, the API fingerprint and all five locked dependency-audit configurations passed. The full Benchmarks workflow passed on the identical reviewed source before merge.An additional
cargo check --locked --workspace --all-features --all-targetspassed without warnings on macOS arm64 and the same source tree. Workspace server dependencies already enable NATS, Wasmtime and other server integrations; this check additionally covers Kafka/Redis combinations and combined generator/test targets. Local types/push crypto regressions passed 806 cases on the patched dependency graph; those overlap the full suite rather than adding an independent 806-test total.The shared native callback correction was tested with three actual weak-lifetime failures before the fix and all seven shared lifecycle cases passing afterward, including physical SQLite WAL-release and callback release after unused, successful and failed initialization. Actual native modules were built on macOS arm64 with Node 26.5.1 and Python 3.14.6: all three Node and three Python hard-exit/restart/retry tests passed, plus three Python weak-reference release states. Those host probes exercised the callback-corrected code before the last dependency-pin refresh; current locked native checks and the shared Rust suites validate the refreshed graph. Python used the README's documented macOS dynamic-lookup linker flags. These are not Node 22 native-runtime claims.
Native PostgreSQL and SQLite, including encryption wrappers, were exercised with real transaction failure injection, lease replacement, rollback and retry scenarios. PostgreSQL timestamp regressions compare complete persisted rows and deliberately supply nanoseconds, retaining original-claim and deletion assertions. The ten ignored cases are two external broker/emulator tests and eight documentation examples. No aggregate line-coverage percentage is claimed.
Dependency exceptions are explicit in
deny.toml: ZIP extraction advisories affect APIs not used by the vendored Swagger build path; the optional UniFFI CLI's unmaintained template dependency is documented separately. The audit also records the pinnedinoutyanked-version warning. These are not silently reported as an empty raw audit.Provider protocol tests use controlled HTTP peers, not live Apple/Firebase delivery. CI/platform builds are not a physical-device or multi-day production soak. Collapse keys preserve the existing 64-bit format; no actual 32-bit runtime execution is claimed.
Follow-up policy
Re-review the relevant boundary when adding a mutable backend operation, a new composite controller, a native callback lifecycle, an event/notification path or an independently leased external effect. Add an actual handoff, mid-write rollback or caller-cancellation case before claiming that boundary safe. Require representative measurements for performance-only changes. Keep schema/client fingerprint checks and all backend variants in CI.
Documentation closure: this report consolidates overlapping ownership/atomicity findings, distinguishes local/merged evidence and measured/unmeasured claims, and links implementation intent to executable regression cases. It does not recommend a documentation portal, architecture rewrite or speculative new infrastructure.
Benchmark and dependency-audit follow-up
Closure audit found that
evaluate_flat_10_stepsconfigured one step and ignored its evaluator outcome. The ten-step invariant reproduced the failure before correction. PR #261 supplies ten matching steps, requires successful evaluator completion and scheduler counts, gives flatten fixtures unique IDs and accurate size labels, verifies saved output retrieval, and adds both Criterion functional smoke targets to CI. All 33 engine and seven storage smoke cases and strict benchmark Clippy passed. The full Benchmarks run39925463401 also passed on benchmark head8ab3c484.That head's CI then caught newly published RUSTSEC-2026-0385 in
ctutils0.4.3. Upstream 0.4.4 fixes constant-time mask generation and 32-bitu128comparisons. The PR now upgrades all four tracked Rust lockfiles to 0.4.4. Adding the missing fuzz-workspace advisory check exposed retained TLS advisories RUSTSEC-2026-0382/0383; its rustls-webpki/root-certs pins now match the patched versions already in main. Its stale jsonschema/serde_ignored manifest requirements were synchronized. CI audits every tracked Rust workspace with--locked, so it rejects uncommitted resolution drift. No advisory exception was added.All five locked audit configurations passed, including default workspace policy, all-feature advisories and independent Node/Python/fuzz graphs. On current head
89747e94529b5178563c1624e30ca11d6f76196d, all 705 types and 101 push library tests passed (806 total, zero failures or ignored), covering encryption, authentication and signing. This is functional regression evidence; upstream provides constant-time verification. The source tree isae988ae56d6d1896add23421b863f02781b42506.PR #261 merged at 09:41:46 UTC as
77c60aeb9d4aeabd0ace5a66348831c68bb65b40, with source treeae988ae56d6d1896add23421b863f02781b42506identical to reviewed head89747e94. Every one of the 22 normal checks and all 23 individual jobs across CI, Mobile and full Benchmarks completed successfully before merge. Actual Linux results: 9,910 passed, zero failed, ten existing ignored cases across 238 groups, plus 40 successful Criterion smoke cases. Strict workspace/all-target Clippy, docs, native bindings, generated clients and all five locked audit configurations passed.New-main CI and Mobile both completed successfully on exact merge commit
77c60ae, with all 22 individual jobs independently verified against the reviewed job matrix. The final repository audit confirms all 33 reported engine fixes are in main; all 13 SDK repositories remain main-only with no open PRs, and the six previously held engine PRs were subsequently closed after the final delegated disposition. Nine SDK main heads have successful recorded checks; four distribution/example heads still have no check records, as listed above. The original user checkout is clean and preserved, and the reviewed worktree is clean.The live OpenAPI and authoring schemas both return HTTP 200. Canonical OpenAPI fingerprint
dc801bbddae427283cf589fb234f3ba7bdcc11d60ab43ab007a26ae0a2b83fc2matches current source, with 248 routes and required authoring fieldsnameandblocks. Publication was last triggered by the API changes in main32569e9a; the seven files in #261 do not match the publication path filters, and the contract remains unchanged. Renamed flatten series start fresh; no throughput improvement is claimed. All completed temporary build caches are removed; source, logs and evidence are retained.Final PR disposition and merged-branch cleanup
Following the final delegated decision, PRs #222, #223, #226, #227, #229 and #230 are closed without merging. The performance proposals lacked representative evidence; the authorization proposals did not establish the claimed reachable flaw; the dependency bumps conflicted with the configured runtime/compiler-lint support. Each PR records its closure reason. There are now zero open engine PRs, and all thirteen SDK repositories also have no open PRs.
Removed 53 proven merged local engine branches, one historical Node SDK branch whose changes were already incorporated by sdk-node#1 and sdk-node#2, and 29 proven merged remote engine branches. Squash-merged aliases were checked against merged PR ancestry or exact main-history trees. Remote deletion used an atomic push with expected-tip leases; recoverable branch-history bundles were retained.
The clean original engine checkout is now on current main
77c60ae. Clean worktree snapshots remain available at detached HEADs. Thereview-fixesbranch/worktree is preserved because it contains uncommitted changes, verified byte-for-byte unchanged;pr182-rebaseretains a unique unmerged commit. Declined, unmerged remote branches were preserved rather than treated as merged work. No source changes were introduced by this cleanup.