Stack 1/4: make accepted closures atomically searchable - #313
Stack 1/4: make accepted closures atomically searchable#313vishaltandale00 wants to merge 21 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Implements #294's task DAG through its join point: #297, #298, #299 and #300. A closure is saved to SQLite and made searchable in Ladybug as one action, or fails with nothing committed to either store. `lbug` 0.18.0 exposes transactions only as BEGIN/COMMIT/ROLLBACK query strings with no prepare/commit split, so two-phase commit is unavailable and true cross-store atomicity cannot be bought at any price. What is achievable is fail-closed ordering, with Ladybug committing first, inside the still-open SQLite write transaction. Reporting failure is truthful precisely because nothing committed. The window between Ladybug's commit and SQLite's stays irreducible; a crash there leaves an extra copy in the derived store, never a missing one. #297 replaces the `--cfg ladybug_qualification` RUSTFLAGS gate with a default-on Cargo feature, so the package job's status certifies something. The cfg was only half the gate: `lbug` omits OpenSSL from its Cargo build metadata, so a build script now derives those link directives from OPENSSL_DIR. Forcing LBUG_BUILD_FROM_SOURCE turned out to be required rather than optional — absent it the crate downloads the newest upstream release, v0.19.1 today, and links that instead of the `=0.18.0` pin the #261 gate certified. The downloaded library reports storage version 43; the reviewed source reports 42. #298 adds migration 0011 and the `SearchIndex` seam. The versions live in SQLite because they must be readable exactly when the store will not open. `SearchIndexRevision` exists because Ladybug has none of its own, so Relayer allocates the number and writes it on both sides; holding it twice is what makes an interrupted write detectable. #299 promotes the #261 contract probe into a real client. Its `p7`/`t41` visibility columns, one boolean per project and thread, cannot work in a real database and became a `published_targets` list. `AcceptedGraphClosure` gained its interaction node, which belongs to no layer and which the root action needs. Closure layers arrive breadth-first, and a relationship write is a MERGE behind a MATCH that silently writes nothing when an endpoint is missing, so nodes go in one pass and relationships in a second. #300 wires the ordering. The per-target mutex orders a target against itself only: SQLite's write lock is global and held across the Ladybug write, so what bounds cross-target stalling is the deadline. Also fixed, from an adversarial review of the ordering: revision allocation cleared SQLite alone and could hand out one number for two different sets of content; an abandoned write leaked an open Ladybug transaction and wedged every later save; and the deadline applied per await rather than across the sequence. And the import path, which no task named, had the same defect the PRD forbids — `import_accepted_conversation` wrote accepted closures with raw SQL and never reached the store. Both accept paths now share one ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AvEcUBSGpcqCNndYiW4rW9
1b5488b to
72d6b77
Compare
A closure was recorded as visible to one target: its project when it had one, its thread otherwise. The frozen query contract's own dataset publishes each node to both — `["project:7", "thread:41"]` — and for good reason. With one target only, a thread-scoped search inside a project would have matched everything in the project, across every other thread in it. That is an authority leak, and it had to be closed before any query path can be built on this. Ordering and publication are now separate ideas. `SearchTarget` remains the ordering key, so a project still advances one revision sequence; `apply` takes the targets the closure is searchable from. `SearchTarget` also spells itself the way the contract does, so what the store holds and what a request names are the same string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AvEcUBSGpcqCNndYiW4rW9
8392d8b to
0a6afe2
Compare
vishaltandale00
changed the title
|
Local #301 checkpoint is complete but intentionally uncommitted because the required repository check is red. Workspace is parent head Graph evidence passes: six-boundary exact-retry matrix, real macOS-arm64 SIGKILL after Ladybug commit, complete topology before/after retry, unpolled commit rollback, 13/13 real Ladybug index tests, and
#301 also remains coupled to #302 for startup reconciliation concurrency, and the per-commit checkpoint latency remains #303's measured gate. |
|
Checkpoint at Implemented #301 crash/reconciliation proof plus the eval-runner cleanup gate repair:
Verification on the exact committed snapshot:
Adversarial assertion: Planck reviewed the exact pre-commit digest above across cleanup ordering, ownership, error preservation, deferred reclamation, and durable teardown. Verdict: certifying, no remaining findings. Earlier findings about deleting state before all owners released and leaking after a failed force-close were fixed and re-reviewed. |
|
Hosted run 33299731931 exposed a gate-scheduling failure, not a product deadline failure: unrestricted cross-file Vitest parallelism started enough full app/server/Ladybug instances that one write exceeded the unchanged 5 s search-publication budget, followed by unrelated polling timeouts. Commit 24c9dbc bounds Vitest to two workers for both npm test and npm run check. It leaves production deadlines, assertions, per-file concurrency, crash tests, and dedicated concurrency tests unchanged. Exact checkpoint evidence:
Hosted confirmation for 24c9dbc remains pending and is not inferred from local evidence. |
|
Hosted run 33300807928 proved that two file workers were still outside the valid gate resource envelope: the same unchanged 5 s Ladybug acknowledgement rejection occurred, followed by seven cross-suite deadline cascades. ARM ASAR packaging passed. Commit 180e440 serializes independent Vitest files with maxWorkers=1. It does not alter production timeouts, assertions, or in-file/native concurrency tests. Exact local evidence:
Hosted confirmation for 180e440 remains pending. |
|
Root cause for hosted run 33301887494 is now source-confirmed: the shared Ladybug connection hard-coded the ordinary 5 s query timeout, while conversation import has a 60 s product deadline. CHECKPOINT therefore returned Interrupted after 5 s on the slower runner even though the import budget remained valid. The durable ordering remains write revision -> COMMIT -> CHECKPOINT -> return; globally widening or removing timeouts is rejected. The fix will propagate the operation absolute expiry into Ladybug writes, use only remaining time for each statement/checkpoint, and restore the ordinary default after settlement. #302 remains the recovery boundary for the irreducible post-COMMIT orphan direction. |
|
Final integrated storage decision changed based on #303 product evidence: per-write forced CHECKPOINT is not viable (200-sample full-ack p95 143.3 ms; 21/200 >=100 ms). Once #302 startup quarantine/rebuild is integrated, immediate post-COMMIT hard-kill damage is recoverable from canonical SQLite. The final boundary will therefore acknowledge after Ladybug COMMIT and immediate searchability, retain operation-scoped 5 s/60 s query budgets, and prove SIGKILL -> detect ahead/unopenable store -> canonical rebuild -> exact retry convergence. This preserves acknowledgement-level freshness without claiming physical checkpoint durability. |
|
Foundation checkpoint at 72b95d5. Integrated #302 rebuild-first recovery and #303 Apple-Silicon latency qualification. SQLite remains canonical; Ladybug is the only search store. Acknowledgement is Ladybug COMMIT plus immediate searchability, with no forced per-save checkpoint and no SQLite search fallback. Focused evidence: lifecycle 17/17 under the ordinary default-parallel command; real SIGKILL 2/2; Ladybug index 13/13; core ordering/crash 12/12. Final 200-save ARM receipt passes the defined p95 gate at 96.2 ms (p50 16.7, p90 34.4, p99 98.2, max 102.6; 2/200 at or above 100 ms). Repository npm run check and npm run build pass on Node 22.23.2. Adversarial review by issue301_adversary certified exact source commit 98977a3 and tree cb53056a1c10e5bfcaa11852689e29bf924ce8db. Reviewed scope: historical rebuild, exact query-visible physical properties and multiplicity, per-target readiness, fsync/rollback durability, one absolute rebuild deadline, malformed inventory recovery, pointer-race confinement under the documented single-primary authority, real SIGKILL convergence, and no SQLite fallback. Verdict: certifying; no unresolved findings. The later commit is receipt-only. Hosted check and macOS ARM package are now the remaining predecessor boundary before PR #319 may advance. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 72b95d5569
ℹ️ 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".
| // may already have committed. The caller fails the write and rolls SQLite | ||
| // back, which lands in the same harmless direction as a crash in the step 3 | ||
| // to 5 window: an extra copy in the derived store, never a missing one. | ||
| let committed = deadline(expiry, write.commit()).await?; |
There was a problem hiding this comment.
When the Ladybug worker-side COMMIT succeeds as this deadline expires, the timeout reports failure and SQLite rolls back, but the worker cannot be cancelled and the subsequently queued rollback is ineffective after the commit. The active index can therefore expose the failed closure as accepted while canonical SQLite still contains drafts, until restart reconciliation or an exact retry; mark the affected target unavailable and reconcile it before serving queries.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
| if !closures.is_empty() | ||
| && let Err(error) = completion::index_and_record( | ||
| self, | ||
| &mut tx, | ||
| target, | ||
| closures, | ||
| crate::publication_targets(metadata.project_id, metadata.thread_id), |
There was a problem hiding this comment.
When finalization succeeds but the product-side receipt, preparation, or publication step subsequently fails, the existing cleanup path deletes the import only from canonical SQLite. Because this new call has already committed its closures to Ladybug, the failed/deleted conversation remains searchable until the application restarts; cleanup must remove or reconcile the derived projection, or indexing must occur after the cleanup boundary.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
| l.published_targets = CASE WHEN l.published_targets IS NULL THEN $targets \ | ||
| ELSE list_distinct(list_concat(l.published_targets, $targets)) END", |
There was a problem hiding this comment.
When one accepted record is reused across project threads whose numeric and lexical ordering differ, such as threads 2 and 10, this union preserves insertion order while canonical_inventory sorts the same targets through a BTreeSet. A rebuild then produces [project:7, thread:2, thread:10] physically but expects [project:7, thread:10, thread:2], causing revisions_match to reject the freshly rebuilt store and preventing graph-server startup; normalize these lists as sets before comparison.
Useful? React with 👍 / 👎.
| "id", | ||
| Value::String(format!("membership:{}:{node_id}", layer.layer.id)), |
There was a problem hiding this comment.
For every accepted layer, this emits membership:<layer>:<content>, but the frozen v1 query contract and fixtures require membership:<layer>:<zero-based-order>:<content>. Queries returning a CONTAINS relationship or path therefore expose a nonconforming stable identity, and the canonical inventory repeats the same faulty format so reconciliation cannot detect or repair it.
Useful? React with 👍 / 👎.
| if let Some(directory) = pkg_config_library_directory() { | ||
| return (directory, Linkage::Dynamic); | ||
| } | ||
| for candidate in ["/opt/homebrew/opt/openssl@3", "/usr/local/opt/openssl@3"] { | ||
| let directory = PathBuf::from(candidate).join("lib"); | ||
| if directory.is_dir() { | ||
| return (directory, Linkage::Dynamic); |
There was a problem hiding this comment.
In the signed desktop path, desktop/release/build-release.mjs invokes Cargo without the prepared OPENSSL_DIR, so this fallback links the default Ladybug-enabled graph server dynamically against pkg-config or Homebrew OpenSSL. Those dylibs are neither bundled nor given an application rpath, so a candidate built on the macOS runner can fail to launch on a clean customer Mac; release builds must supply and require the pinned static prefix mandated by the PRD.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| let post_commit_crash_hook = self.post_commit_crash_hook.clone(); | ||
| Box::pin(async move { | ||
| index.require_ready(target)?; | ||
| let operation = index.runtime.operations.clone().read_owned().await; |
There was a problem hiding this comment.
When a query is queued after apply() but before commit(), both the write and normalized_rows_for hold shared operations guards and run on the same Ladybug connection. The read therefore executes inside the open transaction and can observe an uncommitted closure or a prefix of a multi-closure import, rather than the required prior revision or complete new revision; writes need exclusive serialization against reads or a separate snapshot-capable read connection.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
| if let Err(error) = deadline(expiry, write.apply(closure, published_to.clone())).await { | ||
| let _ = deadline(expiry, write.rollback()).await; |
There was a problem hiding this comment.
When write.apply(...) consumes the operation deadline, this calls write.rollback() under the same already-expired deadline. Constructing that rollback future marks the write settled before any async work runs, while Tokio checks an elapsed timeout before polling the inner future, so no rollback job is submitted and Drop no longer queues one; the Ladybug connection remains in its open transaction and later saves fail behind it until restart. Rollback must be enqueued independently of the expired operation budget.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| "check": "cargo fmt --all -- --check && cargo clippy --workspace --all-targets --all-features -- -D warnings && cargo test --workspace && cargo build -p relayer-app-server -p relayer-graph-server && npm run build:packages && tsc --noEmit && npm run check --workspaces --if-present && vitest run && PYTHONPATH=python/relayer-graph/src python3 -m unittest discover -s python/relayer-graph/tests && npm run lint:ladybug-receipt && npm run prd:check-readability", | ||
| "test": "cargo build -p relayer-app-server -p relayer-graph-server && npm run build:packages && vitest run", | ||
| "check": "cargo fmt --all -- --check && cargo clippy --workspace --all-targets --all-features -- -D warnings && cargo test --workspace && npm run check:graph-crash-reconciliation && cargo build -p relayer-app-server -p relayer-graph-server && npm run build:packages && tsc --noEmit && npm run check --workspaces --if-present && vitest run --maxWorkers=1 && PYTHONPATH=python/relayer-graph/src python3 -m unittest discover -s python/relayer-graph/tests && npm run lint:ladybug-receipt && npm run prd:check-readability", | ||
| "check:graph-crash-reconciliation": "cargo test -p relayer-graph-core --features crash-test-support --test search_index_ordering exact_retry_converges_after_every_completion_crash_boundary && cargo test -p relayer-graph-server --features crash-test-support --test ladybug_crash_reconciliation", |
There was a problem hiding this comment.
The feature-enabled server command selects only ladybug_crash_reconciliation; local cargo test --help defines --test [<NAME>] as “Test only the specified test target.” Consequently, the crash-test-support cases in ladybug_search_lifecycle.rs covering pointer replacement, pre-publication failure, rebuild deadlines, and target isolation are never executed by npm run check, allowing regressions in the newly added reconciliation boundaries to pass CI; include that integration-test target in this command.
AGENTS.md reference: AGENTS.md:L19-L23
Useful? React with 👍 / 👎.
| fn validate_generation_reference(&self, generation: &Path) -> Result<()> { | ||
| ensure_plain_directory(&self.generations())?; |
There was a problem hiding this comment.
If <graph.db>.ladybug is replaced by a symlink containing an otherwise valid active pointer and generation, the production open_reconciled path reaches open_path, whose shared validation checks root/generations and the generation but never the root itself. Startup therefore follows the symlink and later reads, rebuilds, publications, or quarantines data outside the intended database-adjacent directory; move the root symlink/plain-directory check into this shared validation path.
Useful? React with 👍 / 👎.
| Ok(Self { | ||
| storage: SqliteGraphStore::open(path).await?, | ||
| search_index, | ||
| search_index_budget: DEFAULT_SEARCH_INDEX_BUDGET, |
There was a problem hiding this comment.
When path already contains accepted completions and the supplied index is new or stale, this public constructor returns successfully without projecting or verifying the historical closures. Retries of those interactions take the existing-completion early return and never reach the indexing path, while later completions add only themselves, so callers receive a permanently incomplete attached index for the process lifetime; either reconcile here, reject nonempty databases without a reconciled index, or restrict this constructor to fresh storage.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
# Conflicts: # package.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba540c486c
ℹ️ 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".
| const forced = await settle(() => host.forceClose()); | ||
| if (!forced.ok) { | ||
| deferUntilClosingSettles(closing.then(() => {})); |
There was a problem hiding this comment.
When forceClose() rejects and the delayed graceful close also fails, closing.then(() => {}) still resolves because closing was already wrapped by settle; the deferred cleanup at lines 257–259 consequently deletes the working directory. This can occur when a provider's forceShutdown throws and its eventual dispose rejects, leaving the provider process potentially active while its files are removed; defer deletion only after a successful close or otherwise retain the state.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
| until(rebuild_deadline, async { | ||
| let rebuilt = | ||
| build_generation(&layout, &candidate, timeout, &snapshot, rebuild_deadline).await?; |
There was a problem hiding this comment.
When a synchronous startup rebuild times out or fails during construction, reopen, or validation, the error propagates from this block without removing candidate. A graph large enough to exceed the 60-second rebuild budget will therefore leave a potentially large unreferenced generation on every startup attempt and can eventually exhaust disk; wrap the synchronous rebuild like background_rebuild so every pre-publication failure cleans up its candidate safely.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
| // Ladybug 0.18 reports one undirected CONNECTED record in | ||
| // both orientations. An injected duplicate therefore adds | ||
| // two more entries to the physical multiset. | ||
| 2, |
There was a problem hiding this comment.
When any accepted closure contains an authored edge, this adds two canonical inventory entries, but physical_inventory scans CONNECTED with the directed -> form and the product test at ladybug_search_index.rs:228-232 confirms that form returns one record per edge. Consequently revisions_match rejects every freshly rebuilt generation containing an edge, causing synchronous graph-server startup rebuilds to fail and logical rebuilds to mark the affected targets failed; use multiplicity one for this directed physical inventory.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
| # the source build makes it compile the reviewed core bundled inside the pinned | ||
| # crate, with no network access. See docs/evidence/issue-261-ladybug-qualification.md. | ||
| [env] | ||
| LBUG_BUILD_FROM_SOURCE = "1" |
There was a problem hiding this comment.
When a signed release runner has ambient LBUG_LIBRARY_DIR or LBUG_INCLUDE_DIR, setting only LBUG_BUILD_FROM_SOURCE here does not prevent the binding from using those externally supplied native bytes. vendor/ladybug/README.md:20-23 explicitly requires unsetting every manifest override, while desktop/release/build-release.mjs:35-50 forwards the ambient environment unchanged, so an unreviewed prebuilt Ladybug library can be packaged and signed despite the pinned-source claim; reject or remove all environmentMustBeUnset names in the actual release build.
Useful? React with 👍 / 👎.
| if let Some(output) = read_output(database, scope).await? { | ||
| return Ok(output); | ||
| } | ||
| let target = SearchTarget::new(scope.project_id, scope.thread_id); |
There was a problem hiding this comment.
For a project-backed interaction this reduces readiness and ordering to the project target, even though the closure is published to both that project and its individual thread. If reconciliation marks only thread:41 rebuilding or failed—for example because that thread was removed from one record's publication list—project:7 remains ready, so a new save in thread 41 commits and is acknowledged while normalized_rows_for(thread:41, ...) still rejects every query; require readiness for every publication target before accepting the closure, without blocking unaffected project threads.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
vishaltandale00
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf5bb35a5
ℹ️ 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".
| SearchIndexTable::new(&mut *transaction) | ||
| .record_revision(target, committed) | ||
| .await?; |
There was a problem hiding this comment.
If record_revision fails (for example on an SQLite I/O error) or this task is cancelled after index_closures has confirmed the Ladybug commit, SQLite rolls back while the target remains Ready and the failed closure remains searchable. Later writes allocate from max(stored, recorded) and merge new data without removing that orphan, so canonical drafts or an absent import can continue appearing as accepted until restart; mark the target unavailable and reconcile it before serving further queries. This is distinct from the prior commit-deadline finding because the failure occurs after a confirmed Ladybug commit while recording its SQLite receipt.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
1 participant
First PR in the Apple-Silicon graph-search stack for #52. Implements the verified #262 foundation through #300 and records the product decision that macOS Apple Silicon is the first enabled lane.
This PR is deliberately draft. #301 crash reconciliation, #302 readiness/rebuild, and #303 representative-corpus latency remain required before the #262 boundary is integrated. The query parser was moved to the child PR because it belongs to #263.
Preserved product boundaries
Current commits
72d6b77emakes every accepted closure searchable as one atomic action.ddfeb713publishes each closure to its project and thread targets.origin/mainis integrated.0a6afe2erecords the Apple-Silicon delivery lane in the PRD.Exact-head verification
At
0a6afe2e:cargo test -p relayer-graph-core --test search_index_ordering— 11 passed.cargo test -p relayer-graph-server --test ladybug_search_index— 11 passed.npm run lint:ladybug-contract-probe— 20 positive, 27 negative, 15 value, and 9 limit/budget cases inventoried.npm run check— passed, including 133 Vitest files / 1552 tests and all Rust/Python/lint gates.npm run build— passed.The prior hosted
checkfailure was a desktop shutdown timing assertion outside this diff. The focused local boundary and current-main CI pass, but hosted CI must rerun on this head; a repeated failure will be fixed, not waived.Adversarial checkpoint
Reviewers: Hooke, Einstein, and Gibbs. Reviewed the preserved stack, hosted failure, authority/readiness/release boundaries, and planned Prime outcome.
Verdict: the atomic write foundation is suitable as the stack base, but #262 is not complete. The final Prime tool cannot be called usable until reconciliation/readiness, exact executor budgets, real read permits, TypeScript/Python parity, Prime discovery, qualified static linking, and packaged Apple-Silicon submit-then-search evidence pass.
Stack
No merge is requested yet.