Per-table writer queues + per-actor admission + op-kind-aware version check#75
Merged
Conversation
Three new §VI invariants name what OmniGraph commits to as an agent-native system of record: branches as the cross-query coordination primitive, per-query isolation as a per-query opt-in (Serializable up, eventual down), and type-aware agent-resolvable merges. Plus an explicit non-commitments subsection so reviewers see what is intentionally out of scope (Strict Serializable across queries, cross-process linearizable single-object writes, auto-resolution of ambiguous merge conflicts). §VII and §VIII renumber by +3 to make room (35-43 -> 38-46, 44-47 -> 47-50); deny-list and review-checklist references in §IX/§X follow. testing.md's pre-existing stale §VII.33/34/36 references resolve to their actual §VIII.47/48/50 targets in the same pass. staged_writes.rs:866's docstring gains an MR-686 forward reference so the load-bearing concurrency-hazard test points readers at the queue work that closes the gap. §VI.34 is preserved alongside the broader §VI.36 to keep its MR-425 pointer addressable; the overlap is documented in §VI.36's status line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 2 wraps the Omnigraph engine's catalog and schema_source fields in ArcSwap so reads stay zero-cost while apply_schema can swap atomically without &mut self. arc-swap lands as an unused workspace dep here so the follow-up commits that wrap fields can land in isolation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Swap
Bundles the working-tree state from the prior session (PR 0 bench harness,
PR 1a audit_actor_id removal, PR 1b WriteQueueManager + writer integration)
together with the first half of PR 2's interior-mutability foundation
(catalog and schema_source wrapped in Arc<ArcSwap<...>>). The two streams
intermix in 7 of the same files, so splitting via git add -p was
impractical. Subsequent PR 2 steps land as separate atomic commits.
PR 0 — server-level concurrent /change bench harness
- crates/omnigraph-server/examples/bench_concurrent_http.rs (new)
- .context/bench-results/{baseline-main,after-pr1}/ (gitignored)
PR 1a — drop the audit_actor_id field, thread per-call
- removed Omnigraph::audit_actor_id and the swap-restore patterns in
mutation.rs, merge.rs, loader/mod.rs
- actor_id: Option<&str> threaded through MutationStaging::finalize,
mutate_with_current_actor, ingest_with_current_actor,
branch_merge_impl, branch_merge_on_current_target,
commit_prepared_updates*, record_merge_commit,
commit_updates_on_branch_with_expected
- apply_schema and ensure_indices_for_branch pass None (system-attributed)
PR 1b — per-(table_key, branch) write queue + revalidation + sidecar
- new crates/omnigraph/src/db/write_queue.rs with WriteQueueManager,
acquire/acquire_many, sorted+deduped acquisition; 6 unit tests
- Arc<WriteQueueManager> field on Omnigraph + db.write_queue() accessor
- MutationStaging::finalize split into stage_all (Phase A, no queue)
and StagedMutation::commit_all (Phase B, acquire_many + revalidate
pins + sidecar + commit_staged); guards held across publisher
- delete-only mutations now emit recovery sidecars; revalidation
extended to inline_committed tables
- branch_merge_on_current_target, apply_schema_with_lock, and
ensure_indices_for_branch acquire per-table queues for their
touched tables
PR 2 Step B (partial) — catalog and schema_source via ArcSwap
- catalog: Catalog -> Arc<ArcSwap<Catalog>>
- schema_source: String -> Arc<ArcSwap<String>>
- public accessors return Arc<Catalog> / Arc<String>; readers bind
locally where the borrow has to outlive an expression
- new pub(crate) store_catalog / store_schema_source helpers replace
the field assignments in apply_schema and reload_schema_if_source_changed
- 117 tests across lifecycle/end_to_end/branching/runs pass; engine
lib + workspace compile clean
Coordinator wrap (Mutex) and the &mut self -> &self engine API
conversion follow in subsequent commits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the GraphCoordinator field in `Arc<tokio::sync::Mutex<...>>` so engine APIs can move from `&mut self` to `&self` without giving up the coordinator's mutating refresh path. Lock acquisition order: always before runtime_cache (when both are needed in one scope). Critical sections stay short — load+clone for snapshot/version/current_branch, single-method delegations elsewhere. Public API changes: - `Omnigraph::version()` and `Omnigraph::snapshot()` (pub(crate)) become async; callers add `.await`. - `Omnigraph::active_branch()` returns `Option<String>` (cloned) instead of `Option<&str>` borrowed from the coordinator. Callers either `.await` the result + use `.as_deref()`, or hoist into a binding. `&self`-converted methods this round (tied to the coordinator wrap, not the Step C surface refactor): - `swap_coordinator_for_branch` - `restore_coordinator` (now async; was sync) - `sync_branch` - `refresh` - `refresh_coordinator_only` - `reload_schema_if_source_changed` - `branch_create`, `branch_create_from`, `branch_delete`, `branch_list` - `delete_branch_storage_only` - `ensure_branch_delete_safe` - `ensure_schema_apply_idle` - `ensure_schema_apply_idle` helper in schema_apply.rs (matches signature) Caller updates: branch_create_from_impl threads `restore_coordinator`'s new async signature; schema_apply, table_ops, exec/merge wrap every direct `db.coordinator.X()` in `db.coordinator.lock().await.X()`; exec/merge hoists `active_branch_for_keys` once outside the per-table closure that builds queue keys + sidecar pins. All 102 lib tests + 30 branching + 24 runs + 10 lifecycle + 16 staged_writes + 63 end_to_end pass workspace-wide. Zero test regressions; the only behavior change is on the `Omnigraph` API surface (sync -> async on the three accessors above). Step C (engine API conversion: apply_schema, mutate_as, ingest_as, branch_merge_as &mut self -> &self) follows in a subsequent commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The interior-mutability primitives from Step B (catalog ArcSwap,
schema_source ArcSwap, coordinator Mutex, and RuntimeCache's existing
internal locking) make every Omnigraph engine write API safe to expose
under &self. This commit flips the public surface so the HTTP server
can hold Arc<Omnigraph> in PR 2 Step F instead of Arc<RwLock<Omnigraph>>.
Public API conversions:
- mutate, mutate_as
- ingest, ingest_as, ingest_file, ingest_file_as
- load, load_as, load_file
- branch_merge, branch_merge_as
- apply_schema
- ensure_indices, ensure_indices_on
- optimize
Inner functions converted in lockstep (their signatures must match the
new caller shape):
- mutate_with_current_actor, ingest_with_current_actor,
load_direct_on_branch
- execute_named_mutation, execute_insert, execute_update,
execute_delete, execute_delete_node, execute_delete_edge
- branch_merge_impl, branch_merge_on_current_target
- load_jsonl_reader
- schema_apply::{apply_schema, apply_schema_with_lock,
acquire_schema_apply_lock, release_schema_apply_lock,
ensure_schema_apply_idle}
- table_ops::{ensure_indices, ensure_indices_on,
ensure_indices_for_branch, commit_prepared_updates,
commit_prepared_updates_with_expected,
commit_prepared_updates_on_branch,
commit_prepared_updates_on_branch_with_expected,
commit_manifest_updates, record_merge_commit,
ensure_commit_graph_initialized, commit_updates_on_branch_with_expected}
- optimize::optimize_all_tables
- Omnigraph::commit_manifest_updates, record_merge_commit,
commit_updates_on_branch_with_expected, ensure_commit_graph_initialized
The conversion is mechanical: callers that previously took `db: &mut
Omnigraph` now take `db: &Omnigraph`; every interior mutation goes
through the existing locks (coordinator.lock().await, store_catalog,
runtime_cache.invalidate_all). No new locks acquired, no new lock-order
hazards introduced.
102 lib tests + 24 runs + 30 branching + 63 end_to_end + 39 server
tests pass. Workspace compiles clean (1 warning on a now-redundant `mut`
binding in CLI; cleaned up in a follow-up). The remaining work in PR 2
is the AppState flip (Arc<RwLock<Omnigraph>> -> Arc<Omnigraph> +
WorkloadController), the revalidation perf optimization in commit_all,
and the WorkloadController itself.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le (PR 2 Step D) Closes the PR 1b regression (-17% disjoint, -30% same-key) by eliminating the fresh `db.snapshot_for_branch(branch).await` that PR 1b's commit_all issued per mutation. Single-table mutations (`staged.len() + inline_committed.len() == 1`): skip revalidation entirely. The per-(table, branch) Mutex queue holds exclusive while we commit; the publisher's CAS catches any drift that slipped between expected_versions capture and queue acquisition. Conflict cost: 1 orphan Lance HEAD advance, recovered via the existing sidecar protocol on the next ReadWrite open. This is the same trade-off the master plan §"Revalidation perf optimization" prescribes. Multi-table mutations: replace `db.snapshot_for_branch(branch)` (fresh manifest read) with `db.snapshot()` (in-memory). Correct under MR-686's single-process scope because all in-process tenants share one `Arc<Omnigraph>` -> one coordinator; publishes update the shared coordinator BEFORE releasing queue guards, so a contending tenant reads a fresh in-memory view by the time it acquires its queue keys. The within-mutation race (A captures expected_versions[T2]=V0, B publishes T2->V1 during A's stage I/O, A then acquires T2's queue) is caught via the in-memory check. Multi-coordinator deployments (§VI.27 aspirational) would need force-refresh under the queue — documented in §VI's "Explicit non-commitments". Adds a SAFETY comment naming the two load-bearing premises: (1) per-table queue uses exclusive Mutex (not RwLock), and (2) single-coordinator invariant (one Omnigraph engine per process). Migrating either breaks this skip. Regression sentinel `change_conflict_returns_manifest_conflict_409` passes. 102 lib + 24 runs + 16 staged_writes pass with the new path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 2 removes the global server `RwLock<Omnigraph>` (Step F). Without
admission control, one heavy actor would exhaust shared capacity
(Lance I/O threads, manifest churn, network) and starve other actors.
The WorkloadController bounds per-actor in-flight count + bytes and
provides a global rewrite-pool semaphore for compaction / index builds.
New file: `crates/omnigraph-server/src/workload.rs` (~250 LOC + 5 tests).
API:
- `WorkloadController::new(inflight_cap, byte_cap, rewrite_cap)` /
`from_env()` / `with_defaults()`.
- `try_admit(actor_id, est_bytes) -> Result<AdmissionGuard, RejectReason>`
acquires both an in-flight count permit and adds est_bytes to the
per-actor counter atomically; returns RejectReason on either gate.
- `try_admit_rewrite() -> Result<RewriteGuard, RejectReason>` for the
global rewrite pool (Step F maps RewriteGuard exhaustion to HTTP 503).
- `RejectReason::{InFlightCountExceeded, ByteBudgetExceeded,
GlobalRewriteExhausted}`.
Race-free admission via `tokio::sync::Semaphore::try_acquire_owned()`
for the count gate (master plan Finding 6: independent atomic
load+check+add lets two callers both pass a cap-N check; the Semaphore
gate is atomic). Bytes use `fetch_add` + decrement-on-rejection so the
cap is never exceeded even on rollback.
Defaults (override via env):
- OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=16
- OMNIGRAPH_PER_ACTOR_BYTES_MAX=4_294_967_296 (4 GiB)
- OMNIGRAPH_GLOBAL_REWRITE_MAX=4
Tests cover under-cap admission, byte-budget rollback, per-actor
isolation, global rewrite cap, and the load-bearing 32-concurrent-vs-
cap-16 race test (forces real contention via a broadcast release
channel so guards can't recycle permits task-by-task; pins the
master plan's race-free invariant).
Adds workspace dep `dashmap = "6"` for per-actor state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…R 2 Step F) The substantive PR 2 change. Removes the global server `RwLock<Omnigraph>` that has serialized every mutating request across all actors. Disjoint `(table, branch)` writes from different actors now run concurrently, guarded only by the engine's per-(table, branch) write queue (PR 1b) and per-actor admission control (PR 2 Step E). AppState changes: - `db: Arc<RwLock<Omnigraph>>` -> `engine: Arc<Omnigraph>` - New field: `workload: Arc<workload::WorkloadController>` initialized from env (`OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=16`, `OMNIGRAPH_PER_ACTOR_BYTES_MAX=4GiB`, `OMNIGRAPH_GLOBAL_REWRITE_MAX=4`). - `tokio::sync::RwLock` import dropped. Handler updates (16 sites): - All `Arc::clone(&state.db).read_owned().await` and `write_owned()` calls replaced with `let db = &state.engine`. Engine APIs are now `&self` (Step C) so this works directly. - `/export` clones `Arc<Omnigraph>` once and moves into the spawned task instead of acquiring a long-held read lock. - `/change` handler additionally wires `state.workload.try_admit(&actor_arc, est_bytes)`. Cedar runs FIRST so denied requests don't consume admission slots; admission runs SECOND before the engine call. `est_bytes` uses the request body size as a coarse proxy. API surface additions (`api::ErrorCode`): - `TooManyRequests` -> HTTP 429 (per-actor cap exceeded; respect `Retry-After`) - `ServiceUnavailable` -> HTTP 503 (global rewrite pool exhausted) `ApiError` constructors `too_many_requests` / `service_unavailable` and `from_workload_reject` (maps `RejectReason` variants to HTTP status). Other mutating handlers (`/ingest`, `/branches/*`, `/branches/merge`, `/schema/apply`) currently flow through the Arc<Omnigraph> path without admission gates; wiring those is mechanical and lands as a follow-up. The /change hot path covers the bulk of MR-686's load profile. OpenAPI regenerated to include the new ErrorCode variants. 102 lib + 39 server tests + 5 workload tests pass. The regression sentinel `change_conflict_returns_manifest_conflict_409` continues to pass (revalidation perf opt + per-table queue + publisher CAS preserve manifest_conflict semantics under concurrent writers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- docs/server.md: new "Per-actor admission control (MR-686)" section documenting WorkloadController defaults, the 429/503 mapping with Retry-After semantics, the Cedar-then-admission ordering, and the /change-only-for-now scope. Adds 429 / 503 to the listed HTTP status codes and `too_many_requests` / `service_unavailable` to the ErrorCode enumeration in the error model paragraph. - docs/architecture.md: server/CLI diagram updated. Adds WorkloadController and WriteQueueManager nodes; flow is HTTP -> auth -> Cedar -> admission -> engine -> queue. Engine label changed to "Arc<Omnigraph>" to reflect the AppState flip. Prose now points at server.md and runs.md for the admission/queue contracts. The CLI's bypass-admission note is preserved. - docs/invariants.md §VI.23 status annotation: explicitly cites the per-(table, branch) writer-queue + revalidation-under-queue as closing the Lance-HEAD-vs-manifest drift class under concurrent writers once the global RwLock is removed (PR 2 Step F). Continuous in-process rollback recovery still aspirational (MR-870 ticket). scripts/check-agents-md.sh passes (26 links, 26 docs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Step D commit (1b0a2c9) skipped revalidation for single-table mutations, betting that the publisher's CAS would be a no-op under the per-(table, branch) queue. The bench falsified this: expected_versions was captured during stage_all (BEFORE acquire_many), so by the time the queue acquired and the publisher ran, those captured pins were stale w.r.t. any in-process concurrent writer that had published in between. Same-key 8x1 produced ~99% manifest_conflict 409 rejections because every actor after the first carried stale expected_versions. Fix: always re-read the in-memory snapshot under the queue and overwrite expected_versions with the current per-table values. Single-coordinator invariant (one Arc<Omnigraph> per process) makes this safe with zero I/O — publishes update the shared coordinator BEFORE releasing queue guards, so a contending tenant's read sees a fresh view by the time it acquires its keys. The publisher's CAS becomes a correct no-op for queued tables; cross-process drift (coord stale because coord doesn't see external publishes) still rejects via the publisher CAS as ExpectedVersionMismatch -> 409, preserving the change_conflict_returns_manifest_conflict_409 regression sentinel. Trade-off documented in the comment: SERIALIZABLE-opt-in writes (§VI.36 aspirational) will need an additional revalidation step here; the bench's append/upsert pattern is fine because Lance's natural rebase handles concurrent writes onto the same dataset. Bench results captured at .context/bench-results/after-pr2/ + .context/bench-results/comparison.md: - single-actor 1x1: 15.0 ops/s vs baseline 12.3 (+22%) - disjoint 8x8: 7.03 ops/s vs baseline 6.24 (+13%) - same-key 8x1: still rejected (76% errors) by the ensure_expected_version strict check upstream of commit_all; follow-up to address. Disjoint's 13% is below the master plan's ≥8× target. Bench shows the coordinator Mutex is now the dominant serializer; relaxing to RwLock for snapshot/version reads is the next perf step, tracked as a follow-up in comparison.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ch fail) The bdd6440 commit re-captured expected_versions from `db.snapshot()` (bound-branch view). That broke any mutation on a non-bound branch: when the engine handle is bound to main but the mutation targets feature, the bound-branch snapshot returns main's pin for each table, not feature's. The publisher commits to feature, reads feature's manifest entry, sees a different version → 409 even though no concurrent writer existed. Reproduced by `branch_merge_conflict_response_includes_structured_conflicts` which mutates main then mutates feature on the same Omnigraph handle — the second mutation failed with "expected V6, current V5". Switch the re-capture to `db.snapshot_for_branch(branch).await` so the per-branch entries are resolved correctly. This is one fresh manifest read per mutation (the same I/O PR 1b had pre-Step-D), but it is now required for cross-branch correctness — Step D's "in-memory under single-coordinator invariant" rationale was only sound for single-branch workloads. Single-table same-branch mutations could still skip this read (queue exclusivity makes the publisher CAS a no-op), but the conditional adds complexity for marginal gain. Left as a follow-up perf optimization tracked in `.context/bench-results/comparison.md`. Bench numbers updated: - single-actor 1x1: 15.2 ops/s vs baseline 12.3 (+24%) - disjoint 8x8: 7.12 ops/s vs baseline 6.24 (+14%) - same-key 8x1: 77% errors via the strict ensure_expected_version check upstream of commit_all; same-key concurrent-write fix is a separate follow-up. All 102 lib + 39 server + 24 runs + 30 branching + 20 traversal + 9 validators tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix A: op-kind-aware ensure_expected_version. Insert/Merge skip the strict pre-stage check; Update/Delete/SchemaRewrite keep it. New MutationOpKind enum threaded through open_for_mutation_on_branch / open_owned_dataset_for_branch_write / reopen_for_mutation and all callers (execute_insert/update/delete_node/delete_edge, branch_merge::publish_rewritten_merge_table, schema_apply, ensure_indices_for_branch, loader Append/Merge/Overwrite). Closes the 77% rejection rate on same-key concurrent inserts. Fix B: coordinator Mutex -> RwLock. Reads parallelize via .read(); writes serialize via .write(). Atomic-commit invariant preserved by the single .write() covering commit_manifest_updates + record_graph_commit. Bench-as-test change_concurrent_inserts_same_key_serialize_without_409 (server.rs:2180) spawns 12 concurrent /change inserts on a single (table, branch); asserts every request returns 200. Was failing pre-Phase-2; passes post-Phase-2. change_conflict_returns_manifest_conflict_409 (cross-process drift sentinel) and branch_merge_conflict_response_includes_structured_conflicts both still pass. Bench (after-pr2-phase2): - single-actor 1x1: 14.9 ops/s, p50 68ms (baseline 12.3, +22%) - disjoint 8x8: 7.04 ops/s, p50 1023ms (baseline 6.24, +13%) - same-key 8x1: 2.62 ops/s, 0 errors (after-pr2: 77% errors) Disjoint stayed at +13% — Fix B's RwLock helped read paths but the publisher's .write() critical section still serializes graph-wide. Splitting GraphCoordinator into per-concern primitives (manifest in ArcSwap, commit_graph in RwLock, atomic-commit serializer) is the deferred next step. 102 lib + 30 branching + 24 runs + 16 staged_writes + 63 end_to_end + 40 server tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f925ad1739
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+260
to
+263
| let stage_kind = match table.mode { | ||
| PendingMode::Append => crate::db::MutationOpKind::Insert, | ||
| PendingMode::Merge => crate::db::MutationOpKind::Merge, | ||
| }; |
There was a problem hiding this comment.
stage_all now maps every PendingMode::Merge table to MutationOpKind::Merge, which disables the strict pre-stage version check. Update mutations are accumulated as PendingMode::Merge (see execute_update), so an update can be planned on an older snapshot and then reopened/staged against a newer table head without a 409. In concurrent same-branch writes (especially cross-process), this can silently overwrite newer values instead of surfacing a manifest conflict.
Useful? React with 👍 / 👎.
Comment on lines
+915
to
+917
| let (ds, full_path, table_branch) = target_db | ||
| .open_for_mutation(table_key, crate::db::MutationOpKind::Merge) | ||
| .await?; |
There was a problem hiding this comment.
This rewrite path now opens target tables with MutationOpKind::Merge, which skips ensure_expected_version. Because branch-merge publishes via commit_manifest_updates (no per-table expected-version fence), a concurrent external write that advances the target table between merge planning and this open can be merged over silently instead of failing fast. That turns a detectable stale-target race into non-deterministic merge results.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
5 issues found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:2194">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Regression test claims to assert final row count, but only checks HTTP status codes and never verifies the inserts actually landed.</violation>
</file>
<file name="crates/omnigraph-server/src/api.rs">
<violation number="1" location="crates/omnigraph-server/src/api.rs:343">
P2: The doc comment promises a `Retry-After` header that is never actually set in the HTTP response. According to linked Linear issue MR-686, the 429 response should include `Retry-After` so clients can implement proper backoff. Currently the `IntoResponse` impl only emits `(StatusCode, Json(ErrorOutput))` with no extra headers.</violation>
</file>
<file name="docs/server.md">
<violation number="1" location="docs/server.md:58">
P2: The file's existing "Not implemented" section still states "Rate limiting — none" (line 102), which contradicts this new admission-control section that documents 429 responses for per-actor concurrency caps. Update or remove that bullet to avoid confusing readers (the project's maintenance contract in AGENTS.md requires marking stale text).</violation>
</file>
<file name="crates/omnigraph/src/db/omnigraph/schema_apply.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph/schema_apply.rs:446">
P2: TOCTOU: `db.version().await` is called twice — once for the condition check and once inside the error format string. Under interior mutability, a concurrent writer could advance the version between the two calls, producing a misleading error message. Store the first result in a local variable.</violation>
</file>
<file name="crates/omnigraph/src/db/omnigraph.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph.rs:360">
P1: Swapping the shared coordinator and restoring it in separate lock acquisitions is racy under concurrent `&self` callers. Another branch operation can interleave between those steps, run against the wrong branch coordinator, and then have its coordinator state overwritten by the later restore.</violation>
</file>
Linked issue analysis
Linked issue: MR-686: Replace global server RwLock with per-table writer queues and per-actor concurrency caps
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Replace global Arc> with engine: Arc + WorkloadController in AppState | AppState changed to engine + workload fields |
| ✅ | Implement WorkloadController with per-actor in-flight count and byte budget; return 429/503 and env vars | New workload module and API error codes added |
| ✅ | Remove db.read_owned()/write_owned() callsites and adjust handlers to use engine APIs and workload.try_admit after authorize_request | Lib and callsites refactored to use &Omnigraph and workload |
| ✅ | Implement WriteQueueManager with per-(table_key,branch) mutex and acquire_many sorting/dedup; include unit tests | New write_queue module added with mutex per key and tests |
| ✅ | Convert Omnigraph write APIs from &mut self to &self and use interior mutability (ArcSwap, tokio::RwLock for coordinator) | Omnigraph refactored to interior mutability and &self APIs |
| Remove audit_actor_id field and pass audit actor id per-call (kill swap-and-restore pattern) | Refs to audit_actor_id removal implied but not explicit in diffs shown | |
| ✅ | Add MutationOpKind enum and op-kind-aware pre-stage version checks (Insert/Merge skip strict check) | MutationOpKind added and threaded through mutation call sites |
| Split staged writes into two-phase: write_fragments (no manifest) then commit under per-table queue (staging API changes) | Staging refactor present but full table_store two-phase split not visible | |
| Refactor loader to write fragments in parallel, then do a single cross-table commit via ManifestBatchPublisher | Loader adjusted for &self and staging but cross-table commit wiring unclear | |
| ✅ | Schema-apply lock still serializes schema applies (existing test preserved) | apply_schema still acquires schema-apply lock and code preserved |
| ✅ | Manifest conflicts still surface as existing manifest_conflict (HTTP 409) and remain retryable | Conflict sentinel preserved and docs note preservation |
| ❌ | Integration test: two /change requests targeting different (table_key, branch) execute concurrently end-to-end (integration test) | No HTTP integration test found proving disjoint `/change` concurrency |
| A misbehaving actor exceeding its concurrency cap receives 429; other actors are unaffected (test) | Workload unit tests likely exist, but no HTTP-level 429 integration sentinel present | |
| ✅ | Bench: single-table append throughput is no worse than today; cross-table append throughput scales with cores (bench harness & results) | Bench harness added and PR includes bench comparison results |
| ❌ | After the two-phase split: time holding any per-(table,branch) queue lock is <50ms p99 under 16-actor ingest | No p99 queue-hold measurement or test shown in diffs |
| ✅ | OpenAPI and docs updated for 429/503 error codes and server/invariants/architecture docs updated | OpenAPI and docs updated with new error codes and docs sections |
| Cedar policy enforcement runs before the queue lookup so denials short-circuit admission | PR notes enforce-before-admission but explicit wiring not visible in diffs shown | |
| Per-table queue acquisition wired into branch_merge, apply_schema_with_lock, ensure_indices (prepare for recovery MR-870) | MutationStaging.commit_all acquires queues, but other wiring partial/unclear |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| NotFound, | ||
| Conflict, | ||
| /// 429 Too Many Requests — per-actor admission cap exceeded. | ||
| /// Clients should respect the `Retry-After` header. |
There was a problem hiding this comment.
P2: The doc comment promises a Retry-After header that is never actually set in the HTTP response. According to linked Linear issue MR-686, the 429 response should include Retry-After so clients can implement proper backoff. Currently the IntoResponse impl only emits (StatusCode, Json(ErrorOutput)) with no extra headers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/src/api.rs, line 343:
<comment>The doc comment promises a `Retry-After` header that is never actually set in the HTTP response. According to linked Linear issue MR-686, the 429 response should include `Retry-After` so clients can implement proper backoff. Currently the `IntoResponse` impl only emits `(StatusCode, Json(ErrorOutput))` with no extra headers.</comment>
<file context>
@@ -339,6 +339,12 @@ pub enum ErrorCode {
NotFound,
Conflict,
+ /// 429 Too Many Requests — per-actor admission cap exceeded.
+ /// Clients should respect the `Retry-After` header.
+ TooManyRequests,
+ /// 503 Service Unavailable — global rewrite pool exhausted
</file context>
PR 2 made Omnigraph::schema_source() return Arc<String> via ArcSwap, but the failpoints test still compared against &'static str constants. Three E0308 type mismatches were blocking the Test Workspace CI job; this fix restores compilation. - failpoints.rs:125,160,195 now call schema_source().as_str() to align with the &str constants. - Drops 11 unused let mut db = ... bindings on the same path (engine write APIs are &self post PR 2 Step C). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add two durable engineering rules to the maintenance contract so they load into context on every session: - Rule 8: write a regression test that reproduces the bug first, confirm it fails, land it just before the fix commit so the red→green pair is visible in git log. A reviewer can check out the test commit alone and reproduce the failure. - Rule 9: when a bug surfaces, identify the root cause and make the fix correct by construction. Don't patch the symptom. If the design admits the bug class, close the class — don't add a guard around the latest instance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per AGENTS.md rule 8, this commit lands the failing regression test ahead of the fix so the red → green pair is visible in git log. The test asserts the RYW invariant for in-process concurrent UPDATEs on the same row: exactly one writer commits and N-1 receive 409 manifest_conflict. Currently fails on f925ad1 with 1 x 200 + 7 x 500: > "storage: Retryable commit conflict for version 6: This Update > transaction was preempted by concurrent transaction Update at > version 6. Please retry." Lance's transaction conflict resolver correctly detects the Update vs Update race, but the error wraps as `OmniError::Lance(<string>)` and the API surfaces it as 500 internal rather than 409 retryable conflict. Users see "internal server error" for what is documented as a retryable conflict path. The fix lands in the next commit: an op-kind-aware drift check at the commit_all entry that returns 409 ExpectedVersionMismatch for tables whose first touch was Update / Delete / SchemaRewrite when the staged dataset version drifts from the manifest pin under the queue. Closes the bug class "Lance internal conflict surfaces as 500 instead of 409" rather than mapping the specific Lance error variant — the right architectural layer (engine boundary, under the queue) catches the drift before commit_staged ever runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the bug class "Lance internal conflict surfaces as 500 instead of 409" for in-process concurrent strict-op writers on the same row. Pre-fix: in `MutationStaging::commit_all`, after queue acquisition, the staged Lance transaction (built against V0) was handed straight to `commit_staged`. When Lance HEAD has advanced past V0 (because the queue's prior winner already published), Lance's transaction conflict resolver fires `RetryableCommitConflict` for Update vs Update on the same row, which wraps as `OmniError::Lance(<string>)` and the API maps it to HTTP 500. Users see "internal server error" instead of a clean retryable conflict. Fix: track the strictest `MutationOpKind` per touched table on `MutationStaging` and propagate through `StagedMutation`. In `commit_all`'s recapture loop, before each `commit_staged`, fail-fast with `OmniError::manifest_expected_version_mismatch` (→ HTTP 409 ExpectedVersionMismatch) for tables whose tracked op_kind has `strict_pre_stage_version_check() == true` (Update/Delete/SchemaRewrite) when the staged dataset's version doesn't match the fresh manifest pin under the queue. Insert/Merge tables skip the check — concurrent inserts on disjoint keys legitimately coexist via Lance's auto-rebase, so the check would over-reject the existing same-key insert path. Threading: `ensure_path` now takes `op_kind` and stores it in a new `op_kinds: HashMap<String, MutationOpKind>` on `MutationStaging`, with strictness-upgrade semantics so mixed insert+update on the same table still fires the strict check at commit time. `StagedMutation` carries `op_kinds` through to `commit_all`. Pinned by `change_concurrent_updates_same_key_serialize_via_publisher_cas` in `crates/omnigraph-server/tests/server.rs` (added in the previous commit). All Phase 2 sentinels still pass: change_concurrent_inserts_same_key_serialize_without_409, change_conflict_returns_manifest_conflict_409, branch_merge_conflict_response_includes_structured_conflicts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per AGENTS.md rule 8, this commit lands the failing regression test ahead of the fix so the red → green pair is visible in git log. The test demonstrates that two concurrent `POST /branches` calls with distinct `from` parents corrupt coordinator state: A's "operate" step runs against B's swapped coordinator instead of its own, forking the new branch off the wrong parent's HEAD. Currently fails on f925ad1 with all 8 gamma branches (declared parent: alpha, 5 rows) reporting 4 rows — beta's row count. The operate step ran against beta's coord because B's swap interleaved between A's swap and A's operate. Fix lands in the next commit: hold a single `coordinator.write().await` guard across the entire swap-operate-restore sequence in `branch_create_from_impl` so the three steps are atomic relative to other callers. Closes the bug class "non-atomic three-step coordinator manipulation under &self callers" rather than guarding the specific call site — the right architectural seam (single critical section per swap-restore sequence) eliminates the interleave window for branch_create_from and any future swap-restore caller. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the swap-restore race in `branch_create_from_impl` by simply not touching `self.coordinator` at all. Open the source-branch coordinator locally, call `branch_create` on it, drop it. The new branch is durable on disk via the manifest write that `GraphCoordinator::branch_create` issues on its own commit graph; subsequent reads of any coord will see it after their normal manifest refresh. Pre-fix: `branch_create_from_impl` ran swap → operate → restore as three separate `coordinator.write().await` acquisitions. Under `&self` concurrency, two callers with distinct source branches could interleave their swaps, leaving each caller's "operate" step running against the other's swapped coordinator and forking the new branch off the wrong HEAD. Pinned by `concurrent_branch_create_from_distinct_parents_does_not_corrupt_coordinator` (previous commit) which deterministically reproduced the race with 8/8 forks landing on the wrong parent. Why correct by design (AGENTS.md rule 9): closing the bug class "non-atomic three-step coordinator manipulation under &self callers" by removing the manipulation entirely. There's no scratch-space race to lose because there's no scratch space. Note: `branch_merge_impl` at `crates/omnigraph/src/exec/merge.rs:1085-1100` keeps the same swap-restore pattern. Its inner `branch_merge_on_current_target` calls `self.snapshot()` and `self.ensure_commit_graph_initialized()` which acquire the coord lock independently, so the simple "operate on local coord" refactor doesn't compose without a deeper interface change. The per-(table, branch) writer queue inside the merge body (`crates/omnigraph/src/exec/merge.rs:1224`) bounds the damage in practice; a deterministic regression for concurrent merges is tracked under Block 3.1 of the plan. `swap_coordinator_for_branch` and `restore_coordinator` remain crate-internal for now (still used by `branch_merge_impl`); a follow-up can remove them if the merge path is similarly refactored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing change_concurrent_inserts_same_key_serialize_without_409 test claimed in its comment "asserts the final row count equals N" but only checked HTTP status codes. cubic flagged the gap; this commit adds the actual /snapshot read after the concurrent inserts to verify all N batches landed (no silent overwrite) by comparing the post-test node:Person row_count against SEED + N. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per AGENTS.md rule 8, this commit lands the failing regression test ahead of the fix. Currently fails on f925ad1 with 8/8 statuses returning 200 because /ingest does not call WorkloadController::try_admit. The test pins: - /ingest is gated on per-actor admission control (returns 429 when the cap is exceeded). - 429 responses carry the structured `code: too_many_requests` error body so clients can distinguish them from generic conflicts. - 429 responses include a `Retry-After` header so clients can implement bounded backoff. The doc claim at api.rs:343 and lib.rs:344 was that this header exists; the IntoResponse impl currently emits no headers. Two follow-up commits will turn this green: 1. Wire WorkloadController::try_admit on /ingest and the four other mutating handlers (Block 2.1). 2. Emit the Retry-After header on 429/503 responses (Block 2.2). The test uses #[serial] + EnvGuard to override OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1 without racing parallel tests, then spawns 8 concurrent /ingest tasks aligned at a tokio::sync::Barrier so multiple tasks reach try_admit close in time. With cap=1, at least one must be rejected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the gap that admission control only fired on /change. A heavy actor sending bulk-ingest traffic could exhaust shared engine capacity (Lance I/O threads, manifest churn) without hitting the per-actor cap. Wires `state.workload.try_admit(&actor_arc, est_bytes)` into the five remaining mutating handlers AFTER Cedar authorization (so denied requests don't consume admission slots) and BEFORE the engine call. Byte estimates per handler: - /ingest: request.data.len() (NDJSON body) - /schema/apply: request.schema_source.len() - /branches/create, /branches/delete, /branches/merge: 256 (small JSON; the heavy work is bounded per-(table, branch) by the engine's writer queue rather than by request size) The admission guard is held in `let _admission = ...` so it stays alive until handler return, releasing the count permit + decrementing the byte budget on drop. Pinned by `ingest_per_actor_admission_cap_returns_429` (previous commit). The test still fails on the Retry-After header assertion; the next commit emits the header. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the doc-vs-code gap at api.rs:343 and lib.rs:344-355: the documentation claims `Retry-After` is set on TooManyRequests / ServiceUnavailable responses, but `IntoResponse for ApiError` emitted only `(StatusCode, Json(ErrorOutput))` — no header. Wires a constant `RETRY_AFTER_SECONDS = "60"` for both 429 and 503 codes. Plumbing per-RejectReason durations through is a follow-up; the admission rejects we surface today recover bounded by request handler duration rather than calendar wait, so a constant suffices. Pinned by `ingest_per_actor_admission_cap_returns_429`. Test now fully green: 1+ of 8 concurrent /ingest under cap=1 receives 429 with Retry-After: 60. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CTOU Two cleanups bundled because they're both single-line, post-MR-686 hygiene flagged by cubic during PR review: - docs/server.md:102 said "Rate limiting — none" while the new admission-control section earlier in the file documents 429s on the five mutating handlers. Replace with a pointer to the admission section and clarify that no graph-wide rate limiter is wired. - schema_apply.rs:445-451 called `db.version().await` twice — once for the conditional check, once in the error format string — creating a cosmetic TOCTOU under interior mutability. Cache the result in `current_manifest_version` so the error message reflects the version that triggered the rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the cubic acceptance-criteria gap (❌ "Integration test: two /change requests targeting different (table_key, branch) execute concurrently end-to-end"). The bench harness measures the throughput side; this test is the regression sentinel that catches a future change which accidentally re-introduces graph-wide serialization on the disjoint path. Spawns 4 concurrent /change inserts on node:Person and 4 on node:Company. All 8 must return 200, and the post-test row counts on each table must reflect every insert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Future-proofs against MR-895 work that may move or remove the per-(table, branch) writer queue acquisition inside `branch_merge` (`crates/omnigraph/src/exec/merge.rs:1224`). Today the queue linearizes a concurrent /change on main against a `branch_merge feature → main` on the same touched tables; both succeed and the inserted row is preserved post-merge. Codex flagged this scenario as a P1 in PR #75 review claiming the merge could silently overwrite concurrent target writes because the source-rewrite path opens with `MutationOpKind::Merge` (skipping the strict pre-stage check). Validation showed the queue at merge.rs:1224 is held across both Phase B (per-table commit_staged) and Phase C (manifest publish), so there's no interleave window. The Merge op_kind only affects same-process pre-stage drift detection, not cross-write linearization. The test passes on f925ad1; landing it as a regression sentinel catches future changes that drop the queue acquisition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empirical proof of MR-686's central design promise: per-actor
admission control isolates noisy actors from light traffic. The
existing bench_concurrent_http harness measures aggregate throughput;
this harness measures the latency tail seen by light actors while a
heavy actor saturates its own per-actor cap.
Setup: one "heavy" actor flooding /ingest with multi-row NDJSON
batches; N "light" actors each running short bursts of /change
inserts, each authenticating with a distinct bearer token so the
WorkloadController accounts them as separate identities.
Output: heavy throughput / 429 count, light p50/p95/p99/max latency.
Acceptance heuristic on local FS: light-actor p99 < 2 s while the
heavy actor saturates its own cap.
Sample run on local FS, cap=1, 4 light actors x 30 ops, 20 heavy
batches x 50 rows: light p99 = 710 ms, light errors = 0 (well under
the 2 s acceptance target). The test demonstrates the isolation
property — the heavy /ingest holds its own admission slot but
doesn't affect light actors since they have separate per-actor
state.
Usage:
cargo run --release -p omnigraph-server --example bench_actor_isolation -- \
--light-actors 4 --light-ops-per-actor 30 \
--heavy-batches 20 --heavy-rows-per-batch 50 \
--inflight-cap 1 \
--output .context/bench-results/after-pr2-phase2/actor-isolation.json
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| cap: state.byte_cap, | ||
| attempted, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Byte budget check uses saturating_add masking overflow
Low Severity
The byte budget gate uses prior.saturating_add(est_bytes) to compute attempted, but the underlying AtomicU64::fetch_add wraps on overflow rather than saturating. If the counter is near u64::MAX (e.g., due to a bug or extreme load), fetch_add wraps to a small value while saturating_add produces u64::MAX. The cap check passes on the wrapped value, but the actual counter and the checked value diverge, potentially allowing admission when the real budget is exceeded, or preventing the rollback fetch_sub from restoring the correct state.
Reviewed by Cursor Bugbot for commit 8bd9a5f. Configure here.
…serial] Migrates `ingest_per_actor_admission_cap_returns_429` from env-var override to direct `WorkloadController::new(1, ...)` construction via `AppState::new_with_workload`. Removes the `EnvGuard` and the `#[serial]` annotation that paired with it. Why correct by design (AGENTS.md rule 9): the previous round's matrix fix (commit 8bd9a5f) shielded the matrix from this test's env mutation, but the broader bug class — "test A's process-wide env mutation can leak into any test B that calls `AppState::open` / `WorkloadController::from_env()`" — was still reachable by any future test that didn't think to opt out. Closing the class at the source: this test no longer mutates global state at all, so no other test needs to defend against it. Net effect: - This test no longer needs `#[serial]` (was the only reason it was marked) — runs in parallel with the rest of the suite. - The matrix's defensive `with_defaults()` construction (commit 8bd9a5f) remains correct but is no longer required for correctness; it's now a "belt and suspenders" guard against any FUTURE env-mutating test. Verified locally: both tests pass when run together; full server suite (44 tests) green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Cursor Bugbot LOW on commit 3ad359d: try_admit_rewrite is defined and tested but no HTTP handler calls it; the six handler OpenAPI annotations declared status = 503 (added in 8e1a8e7) but try_admit (the only path handlers invoke) returns 429 only. 503 was unreachable. Fix: remove (status = 503, ...) from the six handler OpenAPI annotations and regenerate openapi.json. Kept as forward-looking infrastructure: try_admit_rewrite, global rewrite semaphore, RejectReason::GlobalRewriteExhausted, ApiError::ServiceUnavailable, the 503 branch in IntoResponse, --global-rewrite-cap, and OMNIGRAPH_GLOBAL_REWRITE_MAX. When a future commit wires try_admit_rewrite into a handler, the 503 OpenAPI annotation lands alongside that wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f9a0f31. Configure here.
| cap: self.global_rewrite_cap, | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused try_admit_rewrite not wired into any handler
Low Severity
WorkloadController::try_admit_rewrite is exported as pub and tested, but never called from any HTTP handler. The PR description claims a "Global rewrite semaphore for compaction/index-build paths" but the optimize and ensure_indices engine methods aren't gated by it. The 503 ServiceUnavailable error variant and RejectReason::GlobalRewriteExhausted are dead code in production — only exercised by unit tests.
Reviewed by Cursor Bugbot for commit f9a0f31. Configure here.
| bearer_tokens: Arc::from(bearer_tokens), | ||
| policy_engine: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
new_with_workload hardcodes policy_engine to None
Low Severity
AppState::new_with_workload always sets policy_engine: None, unlike AppState::new which accepts an Option<PolicyEngine> parameter. This makes the constructor unsuitable for any use case that needs both custom workload configuration and Cedar policy enforcement. A future caller reusing this constructor in a production context would silently lose authorization checks.
Reviewed by Cursor Bugbot for commit f9a0f31. Configure here.
Contributor
Author
|
PR description refreshed for the current branch state. Status as of the current HEAD:
Still worth an explicit reviewer decision before merge:
|
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/src/workload.rs">
<violation number="1" location="crates/omnigraph-server/src/workload.rs:119">
P1: Removing rewrite-cap wiring from `WorkloadController` drops global admission control for expensive rewrite paths, which can cause cluster-wide resource saturation under concurrent rewrites.</violation>
</file>
<file name="crates/omnigraph/src/exec/merge.rs">
<violation number="1" location="crates/omnigraph/src/exec/merge.rs:1247">
P1: Post-queue merge revalidation only checks `table_version` and ignores `table_branch`, so branch-state drift can slip through and execute merge logic against stale assumptions.</violation>
</file>
<file name="crates/omnigraph/src/exec/staging.rs">
<violation number="1" location="crates/omnigraph/src/exec/staging.rs:568">
P1: The new inline-committed version-mismatch return happens before sidecar creation, so a conflict can abort after `delete_where` has already advanced HEAD and leave no recovery sidecar for that inline delete.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Linked issue analysis
Linked issue: MR-686: Replace global server RwLock with per-table writer queues and per-actor concurrency caps
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Two `/change` requests targeting different `(table_key, branch)` pairs execute concurrently end-to-end, demonstrated by an integration test. | Added server-level integration test for disjoint /change concurrency |
| ✅ | A misbehaving actor exceeding its concurrency cap receives 429; other actors are unaffected. | WorkloadController implemented and 429 mapped in API/OpenAPI |
| ✅ | The schema-apply lock still serializes schema applies (existing test must still pass). | Schema-apply lock still acquired and lock branch constant preserved |
| ✅ | Manifest conflicts (concurrent same-(table,branch) writes that race past the queue) still surface as the existing `manifest_conflict` error and are retryable by the client. | Manifest-conflict test retained and docs describe 409 behavior |
| Bench: single-table append throughput is no worse than today; cross-table append throughput scales with cores. | Benchmark examples added but no measured results or comparisons provided | |
| ❌ | After the two-phase split: time spent holding any per-(table, branch) queue lock is < 50ms p99 under a 16-actor concurrent ingest load. | No p99 measurements or clear two-phase split instrumentation/results included |
|
|
||
| impl WorkloadController { | ||
| /// Construct from explicit caps. Tests can override. | ||
| pub fn new(inflight_cap: u32, byte_cap: u64) -> Self { |
There was a problem hiding this comment.
P1: Removing rewrite-cap wiring from WorkloadController drops global admission control for expensive rewrite paths, which can cause cluster-wide resource saturation under concurrent rewrites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/src/workload.rs, line 119:
<comment>Removing rewrite-cap wiring from `WorkloadController` drops global admission control for expensive rewrite paths, which can cause cluster-wide resource saturation under concurrent rewrites.</comment>
<file context>
@@ -126,19 +112,15 @@ pub struct WorkloadController {
impl WorkloadController {
/// Construct from explicit caps. Tests can override.
- pub fn new(inflight_cap: u32, byte_cap: u64, global_rewrite_cap: u32) -> Self {
+ pub fn new(inflight_cap: u32, byte_cap: u64) -> Self {
Self {
per_actor: DashMap::new(),
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
| ) { | ||
| continue; | ||
| } | ||
| let expected = target_snapshot.entry(table_key).map(|e| e.table_version); |
There was a problem hiding this comment.
P1: Post-queue merge revalidation only checks table_version and ignores table_branch, so branch-state drift can slip through and execute merge logic against stale assumptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/exec/merge.rs, line 1247:
<comment>Post-queue merge revalidation only checks `table_version` and ignores `table_branch`, so branch-state drift can slip through and execute merge logic against stale assumptions.</comment>
<file context>
@@ -1233,6 +1233,30 @@ impl Omnigraph {
+ ) {
+ continue;
+ }
+ let expected = target_snapshot.entry(table_key).map(|e| e.table_version);
+ let current = post_queue_snapshot
+ .entry(table_key)
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Contributor
Author
|
@cubic-dev-ai review |
@ragnorc I have started the AI code review. It will take a few minutes to complete. |
Contributor
Author
|
@cubic-dev-ai review |
@ragnorc I have started the AI code review. It will take a few minutes to complete. |
Contributor
Author
|
@cubic-dev-ai review |
@ragnorc I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
12 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph/src/db/omnigraph/schema_apply.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph/schema_apply.rs:480">
P2: Schema apply drops authenticated actor attribution by hardcoding `None` for commit/sidecar actor fields.
According to linked Linear issue MR-686, actor identity should be propagated into engine writes; this path loses that attribution.</violation>
</file>
<file name="crates/omnigraph-server/examples/bench_actor_isolation.rs">
<violation number="1" location="crates/omnigraph-server/examples/bench_actor_isolation.rs:249">
P3: `--heavy-batches=0` is currently blocked, but the following error text explicitly tells users to use it to disable heavy traffic.</violation>
</file>
<file name="docs/releases/v0.4.2.md">
<violation number="1" location="docs/releases/v0.4.2.md:26">
P1: According to linked Linear issue MR-686, per-actor admission must cover in-flight queries, but this change documents that read-only endpoints are not admission-gated. That indicates the ticket’s query-admission requirement is not implemented (or the release notes are incorrect), leaving noisy read workloads unthrottled.</violation>
</file>
<file name="docs/architecture.md">
<violation number="1" location="docs/architecture.md:278">
P3: The new server flow diagram implies every HTTP request is admission-gated, but admission is only applied to mutating handlers. This can mislead readers about read-path behavior.</violation>
</file>
<file name="crates/omnigraph/src/db/write_queue.rs">
<violation number="1" location="crates/omnigraph/src/db/write_queue.rs:166">
P2: This test does not validate sorted acquisition order; it will pass even if `acquire_many` stops sorting keys. That leaves a critical deadlock-prevention contract effectively untested.</violation>
</file>
<file name="crates/omnigraph-server/examples/bench_concurrent_http.rs">
<violation number="1" location="crates/omnigraph-server/examples/bench_concurrent_http.rs:106">
P2: According to linked Linear issue MR-686, disjoint-key concurrency must be validated, but `disjoint` mode can silently collide actors onto the same table when `actors > tables`, producing misleading results.</violation>
</file>
<file name="crates/omnigraph/tests/failpoints.rs">
<violation number="1" location="crates/omnigraph/tests/failpoints.rs:354">
P2: This assertion is too weak to prove the delete advanced HEAD; the concurrent update already guarantees `person_head > pre_person_pin`.</violation>
</file>
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:745">
P2: According to linked Linear issue MR-686, per-actor admission should cap in-flight queries, but `/read` and `/export` still run without `workload.try_admit`, so heavy actors can bypass admission and continue starving shared resources.</violation>
</file>
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:2210">
P2: This test expects all updates to race from the same starting version, but the tasks are not synchronized before sending `/change`. That makes the `ok_count == 1` assertion timing-dependent and flaky.</violation>
<violation number="2" location="crates/omnigraph-server/tests/server.rs:3211">
P2: According to linked Linear issue MR-686, this should demonstrate disjoint writes executing concurrently, but the test only asserts 200 responses and final row counts. It can pass even if writes are serialized, so it will miss regressions back to global write-lock behavior.</violation>
</file>
<file name="crates/omnigraph/src/db/omnigraph/table_ops.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph/table_ops.rs:164">
P1: ensure_indices can write to tables that were never queued if they become non-empty after precheck, which breaks per-(table,branch) serialization and recovery coverage under concurrency (MR-686).</violation>
</file>
<file name="docs/invariants.md">
<violation number="1" location="docs/invariants.md:108">
P2: This status text overstates current concurrency safety: delete-only mutations still advance Lance HEAD via inline `delete_where` before queue ownership, so queue+revalidation does not yet fully prevent all same-key concurrent interleavings.</violation>
</file>
Linked issue analysis
Linked issue: MR-686: Replace global server RwLock with per-table writer queues and per-actor concurrency caps
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Two `/change` requests targeting different `(table_key, branch)` pairs execute concurrently end-to-end, demonstrated by an integration test. | Added server-level integration test for disjoint /change concurrency |
| ✅ | A misbehaving actor exceeding its concurrency cap receives 429; other actors are unaffected. | WorkloadController implemented and 429 mapped in API/OpenAPI |
| ✅ | The schema-apply lock still serializes schema applies (existing test must still pass). | Schema-apply lock still acquired and lock branch constant preserved |
| ✅ | Manifest conflicts (concurrent same-(table,branch) writes that race past the queue) still surface as the existing `manifest_conflict` error and are retryable by the client. | Manifest-conflict test retained and docs describe 409 behavior |
| Bench: single-table append throughput is no worse than today; cross-table append throughput scales with cores. | Benchmark examples added but no measured results or comparisons provided | |
| ❌ | After the two-phase split: time spent holding any per-(table, branch) queue lock is < 50ms p99 under a 16-actor concurrent ingest load. | No p99 measurements or clear two-phase split instrumentation/results included |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| actors. | ||
| - **Admission coverage for all mutating handlers**: `/change`, `/ingest`, | ||
| `/schema/apply`, branch create/delete, and branch merge now flow through the | ||
| admission controller. Read-only endpoints are not admission-gated. |
There was a problem hiding this comment.
P1: According to linked Linear issue MR-686, per-actor admission must cover in-flight queries, but this change documents that read-only endpoints are not admission-gated. That indicates the ticket’s query-admission requirement is not implemented (or the release notes are incorrect), leaving noisy read workloads unthrottled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/releases/v0.4.2.md, line 26:
<comment>According to linked Linear issue MR-686, per-actor admission must cover in-flight queries, but this change documents that read-only endpoints are not admission-gated. That indicates the ticket’s query-admission requirement is not implemented (or the release notes are incorrect), leaving noisy read workloads unthrottled.</comment>
<file context>
@@ -0,0 +1,115 @@
+ actors.
+- **Admission coverage for all mutating handlers**: `/change`, `/ingest`,
+ `/schema/apply`, branch create/delete, and branch merge now flow through the
+ admission controller. Read-only endpoints are not admission-gated.
+- **Op-kind-aware version checks**: mutation commit-time drift checks distinguish
+ append-like inserts from strict update/delete work. Inserts remain permissive
</file context>
| // multi-table writers (mutation finalize, branch_merge, future | ||
| // MR-870 recovery). Under PR 1b's intermediate state (global server | ||
| // RwLock still in place), this acquisition is uncontended. | ||
| let queue_keys: Vec<(String, Option<String>)> = recovery_pins |
There was a problem hiding this comment.
P1: ensure_indices can write to tables that were never queued if they become non-empty after precheck, which breaks per-(table,branch) serialization and recovery coverage under concurrency (MR-686).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/db/omnigraph/table_ops.rs, line 164:
<comment>ensure_indices can write to tables that were never queued if they become non-empty after precheck, which breaks per-(table,branch) serialization and recovery coverage under concurrency (MR-686).</comment>
<file context>
@@ -147,13 +154,28 @@ pub(super) async fn ensure_indices_for_branch(
+ // multi-table writers (mutation finalize, branch_merge, future
+ // MR-870 recovery). Under PR 1b's intermediate state (global server
+ // RwLock still in place), this acquisition is uncontended.
+ let queue_keys: Vec<(String, Option<String>)> = recovery_pins
+ .iter()
+ .map(|pin| (pin.table_key.clone(), pin.table_branch.clone()))
</file context>
| .commit_changes_with_actor(&manifest_changes, actor_id.as_deref()) | ||
| .write() | ||
| .await | ||
| .commit_changes_with_actor(&manifest_changes, None) |
There was a problem hiding this comment.
P2: Schema apply drops authenticated actor attribution by hardcoding None for commit/sidecar actor fields.
According to linked Linear issue MR-686, actor identity should be propagated into engine writes; this path loses that attribution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/db/omnigraph/schema_apply.rs, line 480:
<comment>Schema apply drops authenticated actor attribution by hardcoding `None` for commit/sidecar actor fields.
According to linked Linear issue MR-686, actor identity should be propagated into engine writes; this path loses that attribution.</comment>
<file context>
@@ -444,13 +469,15 @@ pub(super) async fn apply_schema_with_lock(
- .commit_changes_with_actor(&manifest_changes, actor_id.as_deref())
+ .write()
+ .await
+ .commit_changes_with_actor(&manifest_changes, None)
.await?;
</file context>
| let qm2 = Arc::clone(&qm); | ||
| let z_clone = z.clone(); | ||
| let a_clone = a.clone(); | ||
| let result = timeout(Duration::from_millis(200), async move { |
There was a problem hiding this comment.
P2: This test does not validate sorted acquisition order; it will pass even if acquire_many stops sorting keys. That leaves a critical deadlock-prevention contract effectively untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/db/write_queue.rs, line 166:
<comment>This test does not validate sorted acquisition order; it will pass even if `acquire_many` stops sorting keys. That leaves a critical deadlock-prevention contract effectively untested.</comment>
<file context>
@@ -0,0 +1,231 @@
+ let qm2 = Arc::clone(&qm);
+ let z_clone = z.clone();
+ let a_clone = a.clone();
+ let result = timeout(Duration::from_millis(200), async move {
+ qm2.acquire_many(&[z_clone, a_clone]).await
+ })
</file context>
|
|
||
| fn pick_table(actor_idx: usize, op_idx: usize, mode: Mode, num_tables: usize) -> usize { | ||
| match mode { | ||
| Mode::Disjoint => actor_idx % num_tables, |
There was a problem hiding this comment.
P2: According to linked Linear issue MR-686, disjoint-key concurrency must be validated, but disjoint mode can silently collide actors onto the same table when actors > tables, producing misleading results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/examples/bench_concurrent_http.rs, line 106:
<comment>According to linked Linear issue MR-686, disjoint-key concurrency must be validated, but `disjoint` mode can silently collide actors onto the same table when `actors > tables`, producing misleading results.</comment>
<file context>
@@ -0,0 +1,267 @@
+
+fn pick_table(actor_idx: usize, op_idx: usize, mode: Mode, num_tables: usize) -> usize {
+ match mode {
+ Mode::Disjoint => actor_idx % num_tables,
+ Mode::SameKey => 0,
+ Mode::Mixed => (actor_idx.wrapping_mul(7919) ^ op_idx) % num_tables,
</file context>
| const SEED_PERSON_ROWS: u64 = 4; | ||
| const N: usize = 12; | ||
|
|
||
| let mut handles = Vec::with_capacity(N); |
There was a problem hiding this comment.
P2: This test expects all updates to race from the same starting version, but the tasks are not synchronized before sending /change. That makes the ok_count == 1 assertion timing-dependent and flaky.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/tests/server.rs, line 2210:
<comment>This test expects all updates to race from the same starting version, but the tasks are not synchronized before sending `/change`. That makes the `ok_count == 1` assertion timing-dependent and flaky.</comment>
<file context>
@@ -2177,6 +2178,1287 @@ async fn change_conflict_returns_manifest_conflict_409() {
+ const SEED_PERSON_ROWS: u64 = 4;
+ const N: usize = 12;
+
+ let mut handles = Vec::with_capacity(N);
+ for i in 0..N {
+ let app = app.clone();
</file context>
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 4)] | ||
| async fn change_disjoint_table_concurrency_succeeds_at_http_level() { |
There was a problem hiding this comment.
P2: According to linked Linear issue MR-686, this should demonstrate disjoint writes executing concurrently, but the test only asserts 200 responses and final row counts. It can pass even if writes are serialized, so it will miss regressions back to global write-lock behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/tests/server.rs, line 3211:
<comment>According to linked Linear issue MR-686, this should demonstrate disjoint writes executing concurrently, but the test only asserts 200 responses and final row counts. It can pass even if writes are serialized, so it will miss regressions back to global write-lock behavior.</comment>
<file context>
@@ -2177,6 +2178,1287 @@ async fn change_conflict_returns_manifest_conflict_409() {
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn change_disjoint_table_concurrency_succeeds_at_http_level() {
+ // HTTP-level pin for MR-686's disjoint-table promise: concurrent /change
+ // requests touching different node types must coexist without admission
</file context>
|
|
||
| 23. **Atomicity is per-query.** Every `.gq` query is atomic — multi-statement mutations are all-or-nothing via the substrate's atomic-commit primitive. No cross-query `BEGIN`/`COMMIT`; branches and merges fill that role for agent workflows. | ||
| *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; and `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case (mutation/load finalize → publisher failure) without restart. The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process recovery for that case requires per-(table, branch) writer-queue acquisition and is the goal of a future background reconciler. Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).* | ||
| *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case under concurrent writers (PR 2 / MR-686) — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case without restart; and the per-(table, branch) writer-queue (`db/write_queue.rs`) + revalidation under the queue (`MutationStaging::commit_all`) prevents concurrent writers on the same key from corrupting each other once the HTTP server's global `RwLock<Omnigraph>` is removed (PR 2 Step F). The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process rollback recovery is the goal of a future background reconciler (MR-870). Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).* |
There was a problem hiding this comment.
P2: This status text overstates current concurrency safety: delete-only mutations still advance Lance HEAD via inline delete_where before queue ownership, so queue+revalidation does not yet fully prevent all same-key concurrent interleavings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/invariants.md, line 108:
<comment>This status text overstates current concurrency safety: delete-only mutations still advance Lance HEAD via inline `delete_where` before queue ownership, so queue+revalidation does not yet fully prevent all same-key concurrent interleavings.</comment>
<file context>
@@ -105,7 +105,7 @@ These are user-visible commitments. They state what the engine guarantees and wh
23. **Atomicity is per-query.** Every `.gq` query is atomic — multi-statement mutations are all-or-nothing via the substrate's atomic-commit primitive. No cross-query `BEGIN`/`COMMIT`; branches and merges fill that role for agent workflows.
- *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; and `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case (mutation/load finalize → publisher failure) without restart. The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process recovery for that case requires per-(table, branch) writer-queue acquisition and is the goal of a future background reconciler. Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).*
+ *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case under concurrent writers (PR 2 / MR-686) — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case without restart; and the per-(table, branch) writer-queue (`db/write_queue.rs`) + revalidation under the queue (`MutationStaging::commit_all`) prevents concurrent writers on the same key from corrupting each other once the HTTP server's global `RwLock<Omnigraph>` is removed (PR 2 Step F). The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process rollback recovery is the goal of a future background reconciler (MR-870). Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).*
24. **Schema integrity is strict at commit.** Type validation, required-field presence (auto-filled from `@default` if declared), uniqueness across batches and versions, and referential integrity — all enforced before commit succeeds. Per-write softening flags are opt-in, never default.
</file context>
Suggested change
| *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case under concurrent writers (PR 2 / MR-686) — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case without restart; and the per-(table, branch) writer-queue (`db/write_queue.rs`) + revalidation under the queue (`MutationStaging::commit_all`) prevents concurrent writers on the same key from corrupting each other once the HTTP server's global `RwLock<Omnigraph>` is removed (PR 2 Step F). The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process rollback recovery is the goal of a future background reconciler (MR-870). Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).* | |
| *Status: upheld at the writer-trait surface, across process boundaries, AND in-process for the common case under concurrent writers (PR 2 / MR-686) — the sealed `TableStorage` trait routes inserts / updates / scalar-index builds / merge_insert / overwrite through `stage_*` + `commit_staged` (Phase A is drift-free); the open-time recovery sweep in `db/manifest/recovery.rs` (sidecars at `__recovery/{ulid}.json` written by `MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) closes the per-table commit_staged → manifest publish residual on the next `Omnigraph::open`; `Omnigraph::refresh` runs roll-forward-only recovery in-process so long-running servers close the common case without restart; and the per-(table, branch) writer-queue (`db/write_queue.rs`) + revalidation under the queue (`MutationStaging::commit_all`) protects staged-write same-key concurrency once the HTTP server's global `RwLock<Omnigraph>` is removed (PR 2 Step F). Delete-only paths still inline-commit via `delete_where` before queue ownership and remain a tracked residual until staged delete support lands. The "Lance HEAD ahead of `__manifest`" drift class is unreachable for op-execution failures, recoverable across process boundaries for all writer kinds, and recoverable in-process for roll-forward-eligible sidecars. Sidecars that would require `Dataset::restore` are deferred to the next ReadWrite open (restore unsafe under concurrency); continuous in-process rollback recovery is the goal of a future background reconciler (MR-870). Two writer paths still inline-commit pending upstream Lance work: `delete_where` (lance-format/lance#6658) and `create_vector_index` (lance-format/lance#6666).* |
| #[tokio::main] | ||
| async fn main() { | ||
| let args = Args::parse(); | ||
| if args.light_actors == 0 || args.light_ops_per_actor == 0 || args.heavy_batches == 0 { |
There was a problem hiding this comment.
P3: --heavy-batches=0 is currently blocked, but the following error text explicitly tells users to use it to disable heavy traffic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/examples/bench_actor_isolation.rs, line 249:
<comment>`--heavy-batches=0` is currently blocked, but the following error text explicitly tells users to use it to disable heavy traffic.</comment>
<file context>
@@ -0,0 +1,392 @@
+#[tokio::main]
+async fn main() {
+ let args = Args::parse();
+ if args.light_actors == 0 || args.light_ops_per_actor == 0 || args.heavy_batches == 0 {
+ eprintln!("--light-actors, --light-ops-per-actor, --heavy-batches must all be > 0");
+ std::process::exit(2);
</file context>
|
|
||
| cli -.-> eng | ||
| srv_in --> auth --> pol --> eng | ||
| srv_in --> auth --> pol --> wl --> eng |
There was a problem hiding this comment.
P3: The new server flow diagram implies every HTTP request is admission-gated, but admission is only applied to mutating handlers. This can mislead readers about read-path behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture.md, line 278:
<comment>The new server flow diagram implies every HTTP request is admission-gated, but admission is only applied to mutating handlers. This can mislead readers about read-path behavior.</comment>
<file context>
@@ -270,13 +270,16 @@ flowchart LR
cli -.-> eng
- srv_in --> auth --> pol --> eng
+ srv_in --> auth --> pol --> wl --> eng
+ eng --> wq
</file context>
</details>
```suggestion
srv_in --> auth --> pol
pol -->|mutating| wl --> eng
pol -->|read-only| eng
devin-ai-integration Bot
added a commit
that referenced
this pull request
May 10, 2026
Previous run failed only on `concurrent_branch_ops_morphological_matrix` in omnigraph-server, a new concurrency stress test introduced by #75. This PR touches only omnigraph-cli; the same test passed on the main branch run immediately before this PR's CI executed. Empty commit to re-trigger CI and confirm flake. Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
devin-ai-integration Bot
added a commit
that referenced
this pull request
May 11, 2026
`concurrent_branch_ops_morphological_matrix` (added by #75) fails ~20% locally on plain main; tracked separately. No code change in this commit. Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
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
1 participant
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
too_many_requestsresponses withRetry-After, injectable workload state for tests/benches, and actor-isolation benchmark coverage./changeconcurrency,/ingestadmission, branch-operation races, branch-op matrix behavior, and inline-delete sidecar conflicts.AGENTS.md.docs/server.mdto say admission gates every mutating handler and removing stale global-rewrite-gate wording.AGENTS.mdrule to keep the always-loaded guide short because added lines carry recurring context-window cost.Review & Testing Checklist for Human
staging.rsandmerge.rs; both intentionally prefer safe 409 over silent overwrite under concurrent target movement.staging.rs; sidecars are now written before the inline manifest-version check can reject after Lance HEAD moved.OMNIGRAPH_GLOBAL_REWRITE_MAX/service_unavailablematches the intended public API for this PR.docs/releases/v0.4.2.mdas the public v0.4.2 release notes and confirm it now covers the full branch/PR scope without private tracker references.Notes
Local verification run:
cargo check --workspace --all-targetscargo test -p omnigraph-engine --features failpoints --test failpoints inline_delete_conflict_writes_sidecar_before_rejecting -- --nocapture(red state was first confirmed with the old sidecar ordering: missing sidecar under__recovery/; green state passes after this fix)cargo test -p omnigraph-engine --features failpoints --test failpointscargo test -p omnigraph-engine --test runscargo test -p omnigraph-server --test server change_concurrent_updates_same_key_serialize_via_publisher_cascargo test -p omnigraph-server --test server change_concurrent_inserts_same_key_serialize_without_409cargo test -p omnigraph-server --test server change_disjoint_table_concurrency_succeeds_at_http_levelcargo test -p omnigraph-server --test server change_conflict_returns_manifest_conflict_409cargo test -p omnigraph-server --test server matrixcargo test -p omnigraph-engine --test branchingcargo test -p omnigraph-engine --test composite_flowcargo test -p omnigraph-server --test server concurrent_branch_merges_distinct_targets_do_not_swap_into_each_othercargo build -p omnigraph-server --examplescargo test -p omnigraph-serverOMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapicargo test --workspace --tests --no-fail-fastscripts/check-agents-md.shAdditional v0.4.2 release verification:
grep -R "MR-|Linear|linear|INT-" docs/releasesreturned no matches after cleanup.OMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapipassed.cargo test -p omnigraph-engine --features failpoints --test failpoints inline_delete_conflict_writes_sidecar_before_rejectingpassed.cargo test -p omnigraph-server --test server change_conflict_returns_manifest_conflict_409passed.Devin Review docs-finding verification:
scripts/check-agents-md.shpassed.cargo check -p omnigraph-server --all-targetspassed.git diff --checkpassed.Full-scope release-note and AGENTS follow-up:
docs/releases/v0.4.2.mdto cover the earlier queue/admission/branch-race/deadlock work as well as the later correctness fixes and release prep.grep -R "MR-|Linear|linear|INT-" docs/releasespassed.scripts/check-agents-md.shpassed.git diff --checkpassed.cargo test -p omnigraph-server --test serverwas rerun after one transient matrix-cell sentinel 409 and passed on retry;cargo test --workspace --tests --no-fail-fastalso passed the full server test suite.OMNIGRAPH_S3_TEST_BUCKETwas unset in this session, so the optional S3 test was not run.Link to Devin session: https://app.devin.ai/sessions/eeb5028a6e554c388af5f2dc5a307db5
Requested by: @ragnorc