feat(storage): add default-unused atomic system-record apply - #2124
Open
Jurij89 wants to merge 2 commits into
Open
feat(storage): add default-unused atomic system-record apply#2124Jurij89 wants to merge 2 commits into
Jurij89 wants to merge 2 commits into
Conversation
11 tasks
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
Contributor
Author
|
@otReviewAgent please retry the review on head |
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
Contributor
Author
Final adversarial review: convergedI ran independent transaction/recovery, concurrency/liveness/resource, and architecture/stack/evidence passes against head Findings resolved
Verification
The PR is converged for its explicitly default-unused merge boundary. The activation gates already listed in the PR remain mandatory before a production issuer/opener is wired. |
Jurij89
added a commit
that referenced
this pull request
Aug 7, 2026
…ove we own Round-3 HIGH, confirmed and reproduced: the scheduler holds ordinary work while a control barrier is pending and releases it when the barrier settles -- IDENTICALLY whether the barrier resolved or rejected -- and no mutation path consulted ownership at dispatch. So a mutation queued before a generation handoff resumed after that handoff FAILED. Measured: an `INSERT DATA` went on the wire while the store's own lease already read `terminal: true, port-release-unproven`. Every fact needed to refuse was available at the dispatch instant and never read. **This is not the fix the review asked for, and the difference is deliberate.** The review asked to move B3 (#2124)'s armed mutation latch down into B2. I measured that latch instead of copying it, and it should not come down: 1. Its safety margin is ZERO microtasks. The scheduler rejects the barrier one reaction before it releases queued work, so a latch written synchronously in the lane's `catch` wins by exactly one tick. Delaying that write by a single `await` -- an awaited log, a metric, a future refactor of `startBarrier` to `.finally().then()` -- flips the guard off with every test still green. Measured directly: at N=0 extra awaits the queued write sees `latched=true`; at N=1 it sees `latched=false`. 2. It is blanket. It arms from one `catch` covering the whole barrier, but of the five handoff steps two -- `destroyClient` and `rotateMaterializationEpoch` -- leave the child proven and still listening. Latching there refuses every managed write for the process lifetime on a node where nothing was wrong. 3. It does not implement the review's own requirement (4). There is no clear site anywhere: `managedMutationFailure ??= reason` and nothing resets it. The clean-generation-start failure is EXPLICITLY recoverable -- the supervisor SIGKILLs the unproven child and `scheduleRevive`s -- so a permanent latch converts a self-healing respawn into a mandatory node restart. That is worse than the harm prevented, in the more likely of the two failure modes. 4. It fixes the RAREST instance of the class. The same exposure exists with no lane involved: on an ordinary child exit the supervisor invalidates ownership and revives, and for the whole backoff the adapter keeps POSTing to a port it does not own. That path ships today. The lane path has NO production caller. So this is a dispatch-time READ of the live lease, not an armed latch: - no arming, therefore no ordering to get wrong -- point 1 dissolves; - keyed on proven liveness rather than "the barrier threw", so a failure that left the child healthy refuses nothing -- point 2; - self-clearing by construction, because there is no state to clear: when the supervisor binds a proven replacement the lease reads live again -- point 3, and it satisfies requirement (4) more strictly than a latch could; - it covers the child-exit window too, because that also leaves the lease not-ready -- point 4. Placement is the whole guarantee, and it has its own mutant. The check is INSIDE `postUpdate`'s work callback -- after the scheduler admits the write -- not at the top of `insert()`. Every mutation funnels through `postUpdate`, so one site covers `insert`/`delete`/`deleteByPattern`/`deleteBySubjectPrefix`/`update`/ `dropGraph`/`replaceGraph`/`replaceGraphAndSubject`/`replaceSubject`. Moving it to call time leaves every other test green and reddens exactly the barrier-queued case, which is the case the review reported. Scope, stated rather than assumed: - MUTATIONS only. Refusing reads would turn every child respawn into a total store outage -- the same reasoning that made a failed lane shutdown leave the child alive rather than kill the daemon's store. - Managed stores only. An operator-configured store has no lease, returns on the first line, and pays one already-loaded field read. - `isManagedOxigraphOwnershipLiveV1` is allocation-free on purpose; `readManagedOxigraphOwnershipSnapshotV1` freezes a new object, so it is called only to build the error, on the cold path. **One guard was written and then removed.** The predicate started as `ready && !terminal`; the `!terminal` term's solo mutant SURVIVED. Terminality implies not-ready in both directions -- `invalidate()` clears `ready` on every reason, and `bindReadyGeneration()` throws on a terminal lease -- so the second term could never change an outcome. It reads as extra protection, so it is gone. Evidence: 7 new tests, all asserting on RECORDED REQUESTS rather than on the thrown error, because the property is zero I/O and an error raised after a socket opened would satisfy `rejects.toThrow` while failing the actual requirement. Both regressions the review names are covered, plus the no-lane child-exit case, the recovery case, the unmanaged-store case and reads-still-served. Solo-mutant sweep: 5 of 5 killed, including the placement mutant. 218/218 across the eleven B2 storage suites; live gate `PASS: 27 checks`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
10 tasks
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
2 participants
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.
Summary
Related
feat/2052-system-record-materializer).feat/2052-system-record-core).Diagrams
Verified atomic apply boundary
Before:
sequenceDiagram participant Verifier participant Registry participant Lane participant Scheduler participant Oxigraph Verifier->>Registry: Verified replacement Registry-->>Verifier: No atomic apply route Verifier-->>Lane: Not callable Lane-->>Scheduler: No system-record transaction Scheduler-->>Oxigraph: No system-record writeAfter:
sequenceDiagram participant Verifier participant Registry participant Lane participant Scheduler participant Oxigraph Verifier->>Registry: consumeCandidate(signed closure, bundle, slice) Registry->>Lane: One-shot proof and 12 MiB lease Lane->>Scheduler: Generation-bound exclusive agents admission Scheduler->>Oxigraph: Bounded exact inspection Oxigraph-->>Scheduler: State, claims, capacity, projection Scheduler->>Oxigraph: One conditional SPARQL update Oxigraph-->>Scheduler: Response or uncertain transport outcome Scheduler->>Oxigraph: Exact post-read Scheduler-->>Lane: Applied, deferred, or indeterminate Lane-->>Verifier: Typed terminal outcomeDefault production path
Before:
sequenceDiagram participant Caller participant Registry participant Store participant Oxigraph Caller->>Store: Ordinary query or mutation Store->>Oxigraph: Legacy dispatch Oxigraph-->>Caller: Result Registry-->>Caller: No production proof issuerAfter:
sequenceDiagram participant Caller participant Registry participant Store participant Oxigraph Caller->>Store: Ordinary query or mutation Store->>Oxigraph: Same legacy dispatch while admission is disabled Oxigraph-->>Caller: Result Registry-->>Caller: No production proof issuerFiles changed
packages/core/src/system-record-*.ts,packages/core/src/ka-bundle-v1.tspackages/storage/src/system-record-*-v1-internal.tspackages/storage/src/system-record-materializer-v1.tspackages/storage/src/adapters/managed-http-client.ts,packages/storage/src/adapters/sparql-http.tspackages/storage/src/managed-oxigraph-ownership-v1-internal.ts,packages/cli/src/daemon/oxigraph-server.tspackages/{core,storage,cli}/test/**devnet/issue-2052-managed-ownership/run.ts,.github/workflows/system-record-managed-ownership.ymldocs/adr/0002-system-record-sync-v1.mdActivation gates
This PR is mergeable only as default-unused infrastructure. It is not evidence that the system-record lane can be activated. Before any production proof issuer/opener is wired, follow-up work must prove all of the following:
Until those gates pass, no production issuer/opener may be added and legacy synchronization remains authoritative.
Test plan
pnpm --filter @origintrail-official/dkg-core buildpnpm --filter @origintrail-official/dkg-core test:system-record- 60 passed, 2 skipped exhaustive cases.pnpm --filter @origintrail-official/dkg-storage build.github/workflows/system-record-managed-ownership.yml- 310 passed.vitestfor managed HTTP, materialization epoch adapter, and SPARQL HTTP/blank-node compatibility - 71 passed.pnpm --filter @origintrail-official/dkg buildvitestforoxigraph-managedandoxigraph-server- 63 passed.pnpm test:live:system-record-managed-ownership- PASS, 27 checks across 3 predecessor entries.git diff --checkThe live workflow proves owned-child lifecycle and predecessor compatibility only. It does not dispatch a verified atomic apply and must not be cited as the missing maximum-size/lost-response activation evidence.