Skip to content

fix(storage): re-issue and read back conditional control-object writes - #1176

Open
ragnorc wants to merge 4 commits into
ModernRelay:mainfrom
ragnorc:fix/control-object-reissue
Open

ragnorc wants to merge 4 commits into
ModernRelay:mainfrom
ragnorc:fix/control-object-reissue

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Implements the control-object part of §2 of the accepted RFC, storage outcomes and restartable ownership. Conditional control-object writes now resolve an ambiguous answer by re-issue and read-back, as manifest commits already do (#1092). A definite refusal no longer leaves its key unsettled.

Storage adapter

write_text_if_absent and write_text_if_match used to return on the first error. They now resolve each answer through omnigraph_storage::conditional. That module is the one policy for a conditional request whose answer can be lost, and the manifest commit handler uses it too. It owns the re-issue count, the pauses, the failure classification and which answer ends a request. Each answer is classified:

  • Conflict (412, a create's "already exists", an if-match on an absent object).
  • Refused before applying: a missing bucket or container, an invalid path, an unsupported operation, or the owning scope's own refusal. A permission or authentication error is a refusal only on a single-attempt client, such as a scope's. An unscoped S3 or Azure client repeats requests itself, so its 403 or 401 can follow an earlier attempt that applied. The first copy's refusal returns at once.
  • Ambiguous: anything else. The request is re-issued unchanged, up to CONDITIONAL_REQUEST_REISSUES (four) times, with pauses of 0.2 s doubling to 1.6 s. A refused re-issue is re-issued too.

The key is read back after either of these:

  • a conflict that an earlier copy of the request may explain;
  • the last lost or refused re-issue.

Reading the key back resolves the write:

  • Equal bytes mean this request landed. Every caller's payload identifies its writer, or identical bytes are the same effect: lock records and the unprepared init claim carry ULIDs, a lease beat is a sequence in its owner's directory, and state.json and content-addressed resources are the effect. The trait text states the requirement. The prepared graph-create claim did not meet it (see below).
  • A key that shows the precondition failed is a definite conflict.
  • A key that still satisfies the precondition (absent, or at the expected ETag) is an unresolved error.

An unscoped S3 or Azure client retries internally, so its first conflict is read back too: it may be answering its own earlier copy.

S3 answers 409 while another conditional write to the key is in flight, and object_store reports it as "already exists" (client/retry.rs). That earlier write can be our own copy, so:

  • a create conflict over an absent key is unresolved, not Ok(false);
  • an if-match 409 is re-issued.

On a local root an error is a refusal. LocalFileSystem finishes a put before it answers, and without fsync (its default) nothing follows the publishing link or rename. Local behaviour is therefore unchanged.

Storage scope

  • Definite refusals settle. A request refused before applying settles its key instead of leaving it unsettled. Every request that can leave a key unsettled goes to the single-attempt client, so no earlier copy of it was sent.
  • Unsettled keys record their precondition. Each unsettled key records the precondition that left it there (None for an unconditional request). A request with the same precondition, the owner's re-issue, is admitted; every other request stays refused.
  • Only a re-issue's success settles the key. Its conflict does not, because of the 409 case above. The adapter settles the key from its read-back once the precondition visibly no longer holds (ABA accepted, as the RFC accepts it).
  • Routing is unchanged. Conditional requests still go to the single-attempt client, and their owner re-issues them.

Prepared graph-create claim

A prepared claim was a digest of the token, so every apply of one token wrote the same bytes, and the read-back could tell two initializers that both held the claim. Each apply's claim now names the apply. Version 3 carries the token digest, graph commit id, root and a fresh attempt_id. settle_prepared_graph_create_after_quiescence recognizes the token's claim by the first three, whichever apply wrote it, and still accepts the exact version-2 claim earlier releases wrote.

Manifest commits

The manifest handler (lance_clone) now runs on the shared policy, and its classification is unchanged: it cannot tell Lance's provider from a scope's client, so it takes the client to repeat requests. One outcome changes. A re-issue that meets a held version is recorded Unresolved, not Held. On S3 that 409 can answer the earlier copy still in flight, and Held let Lance treat a missing version as definitive and delete the transaction file that copy names. The first copy's conflict stays Held.

Callers

The beneficiaries are the cluster lock and release marker, the init claim and create-if-absent probe, deployment resources, state.json, and lease beats and claims.

The lease keeper fences on its own timer, so a beat that re-issues inside the adapter cannot postpone the fence. The 409 handling also stops a lease beat or claim from counting a copy still in flight as present.

Tests

  • scoped::tests::a_conditional_reissue_is_admitted_issue_1067
  • scoped::tests::a_refused_request_leaves_its_key_settled_issue_1067
  • tests::write_text_if_absent_resolves_lost_answers_issue_1067
  • tests::write_text_if_match_resolves_lost_answers_issue_1067
  • tests::repeating_client_reads_back_its_own_copy_issue_1067
  • tests::scoped_conditional_writes_settle_by_reissue_issue_1067
  • tests::the_client_decides_what_a_failed_copy_says_issue_1067: a local error is not re-issued; a repeating client's permission error is re-issued, a scope's is refused; an unscoped first 409 over an absent object stays unresolved; and equal bytes read back as this writer's.
  • conditional::tests: pauses, the single-attempt rule and which answers end a request.
  • lance_clone::tests::manifest_request_is_re_issued_until_the_store_answers_issue_1067: a first conflict is held, and a re-issue's conflict is unresolved.
  • db::omnigraph::tests::one_prepared_token_has_one_init_claim_owner_issue_1067: two claims of one token through an adapter that reads a create's conflict back by bytes; the second is refused. Settlement recognizes any apply's claim and the legacy version-2 claim, and refuses another token's.

The adapter tests use a scripted store that applies-then-loses, loses before applying, refuses, answers 409, lets another writer in first, or repeats a put. They run on paused tokio time.

Docs

  • docs/user/operations/storage-failures.md: Retries and Logs.
  • docs/dev/control-plane.md: the scope and adapter rules, and the shared policy.
  • docs/dev/lance.md: the manifest re-issue's conflict, and the first-copy limit.
  • docs/dev/recovery.md: the prepared claim's attempt id.
  • Changelog note control-object-writes-reissue-lost-answers.fixed.md.
  • The RFC's §2 control-object bullet names this PR and records the 409 refinement.

Verification

At 2aaae98: the six-package suite (engine, server, cluster, core, storage, catalog, with the failpoint features) passed every binary, and cargo fmt --all --check, both workspace clippy graphs, check-docs.py and typos were all clean.

At 94691fa, after the review:

  • the storage crate's 90 unit tests;
  • the focused issue_1067, conditional::, manifest_request and init-race tests across storage, core, catalog and engine (43 tests);
  • clippy on those four packages with -D warnings. The local toolchain is 1.98 and the repository pins 1.97.1. Two 1.98-only lints, on code this PR does not touch, were allowed locally; CI's 1.97.1 gate covers them.
  • cargo fmt, check-docs.py and typos.

CI runs the workspace suite.

  • DST was not run. Its storage wraps ObjectStorageAdapter::in_memory(), whose only errors are conflicts, and it injects faults above the adapter, so neither its paths nor the cost golden change.

write_text_if_absent and write_text_if_match re-issue an ambiguous answer
unchanged, up to four times, and return a refusal at once. After a
conflict an earlier copy may explain, a refused re-issue or the last lost
answer, they read the key back: equal bytes are this request's, a key
that shows the precondition failed is a conflict, and one that still
satisfies it is unresolved. An unscoped S3 or Azure client retries
internally, so its first conflict is read back too. A local filesystem
error is a refusal.

The storage scope settles a request the store refused before applying
it. An unsettled key records the precondition that left it there and
admits its owner's re-issue; only the re-issue's success settles it,
since S3 answers 409 (reported as already-exists) while the earlier copy
may still be in flight. The adapter's read-back settles the rest.
@ragnorc
ragnorc marked this pull request as ready for review October 26, 2026 11:36

ragnorc commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Recommendation: revise.

Why

A lost answer on a conditional control-object write failed on the first error. A create that did land was not recognized. The key could stay unsettled for the life of the server.

What

write_text_if_absent and write_text_if_match classify each answer as a conflict, a refusal, or an ambiguous answer. An ambiguous answer is re-issued up to four times, with the same pauses as a manifest commit. The key is then read back.

How

A precondition failure, a create that finds the object, and an if-match on a missing object are conflicts. A local error is a refusal, because the local store finishes the put before it answers. Any other error is ambiguous. An unscoped S3 or Azure client retries inside object_store (default max_retries is 10), so the first conflict is also read back. Equal bytes are reported as this call. The storage scope admits only the same precondition on a re-issue, and it settles the key from that read-back.

Impact

A lost acknowledgement can now resolve for a writer-unique lock, a lease beat, and a state or resource object whose bytes are the effect. It cannot resolve exclusive ownership for a payload that two callers share. The prepared graph-create claim is that payload.

Blocking

Equal bytes are treated as "this call created the object" (ObjectStorageAdapter::write_text_if_absent, the read-back arm). The trait text says contents must identify its writer. PreparedGraphCreate::claim_payload does not. It is a digest of the prepared token. Every caller of one token writes the same bytes.

acquire_init_claim treats Ok(true) as exclusive ownership and then runs init_commit_phase. The comment on that function calls it the one cross-process init claim. apply_prepared_graph_create builds an unscoped adapter with storage_for_uri. On that adapter conditional_requests_may_repeat is true for S3 and Azure, so the first AlreadyExists is read back. A second apply of the same token gets Ok(true), and both initializers proceed.

I ran a temporary case at this head and then removed it. An unscoped S3 adapter, a foreign object with the same bytes, and one put returned Ok(true).

The same comparison applies after any ambiguous answer, including a scoped single-attempt client. A second initializer whose answer is lost sees the first initializer's identical claim and also gets Ok(true). A clean conflict on a scoped client still returns Ok(false), because that client does not retry internally. The cluster path uses graph_storage, which is scoped when the owner has an I/O scope, so the clean-conflict case stays exclusive there. The lost-answer case does not.

The unprepared init claim carries a ULID and stays exclusive. Lock and bootstrap payloads carry a unique lock id. state.json and content-addressed resources are the same effect when the bytes match. That matches the rule those keys need.

The if-match read-back uses the same byte test (write_text_if_match). That test is sound for a unique lock record and for state.json, where the desired bytes are the effect. The create path, used as a lock, is the break.

Two initializers can both observe success. That breaks the loud-integrity rule for the exclusive claim. The lost-ack failure this change fixes is the other class: a request that landed must not be reported as a hard failure, and a retry must stay bounded. The adapter is the right owner for that class. Byte equality does not hold exclusive ownership for a shared payload.

Change one of these, and add a regression next to the issue_1067 tests:

  • Return Ok(true) from a read-back only when this call applied the object, or when the payload identifies the writer. A shared payload that is already present must stay Ok(false).
  • Give the prepared claim a writer-unique id, and teach settlement to recognize the digest rather than the whole document.

A lost acknowledgement of a unique payload can still return Ok(true). A second caller of a shared payload must stay excluded.

Optional

  1. The re-issue budget is a second copy. CONDITIONAL_REQUEST_REISSUES and conditional_reissue_pause repeat MANIFEST_REQUEST_REISSUES and reissue_pause. The pauses match for attempts 1 through 4 and differ at attempt 5 (1.6 s against 3.2 s). Classification already disagrees. Control objects treat PermissionDenied and Unauthenticated as refused (refused_before_applying). Manifest classification treats those as ambiguous, because a retrying client can apply and then fail authentication. The scope comment says every unsettled request uses a single-attempt client. Unscoped S3 and Azure clients do not: max_retries defaults to 10. A 5xx retry can apply, and a later 403 or 401 returns with no read-back. The next call with the same payload still recovers through AlreadyExists. One resolver should own the count, the pause, and the classification, with an explicit single-attempt exception. Five more conditional methods would copy this loop again.

  2. The user page says an S3 or Azure control-object write is re-issued up to four times (docs/user/operations/storage-failures.md). That is the adapter loop. An unscoped S3 if-match also sets retry_on_conflict(true) in object_store 0.14.2, so each of those attempts can retry HTTP 409 up to the client max_retries (10 by default). Those inner retries are not in the re-issue log. A scoped client sets max_retries to 0, so the server path matches the page and RFC section 3. RFC section 3 sends conditional requests to the single-attempt client and lets only the resolver re-issue them.

  3. The scripted store matches the error variants (AlreadyExists for 409, PermissionDenied, a generic lost answer). It does not show three store facts: a local generic error is not re-issued, an unscoped first 409 with the object still absent stays unresolved, and a second writer with equal bytes returns Ok(true). Azurite reports a failed if-match as 409, which object_store maps to AlreadyExists, and this code re-issues it. Real Azure reports 412, which is an immediate conflict. The final read-back is still the same. GCS is not a supported scheme. An S3-compatible store that ignores If-None-Match (the Lance object-store guide names Garage) can let a re-issue overwrite. Manifest commits already depend on atomic conditional puts.

Checked, and left as sound

The lease keeper selects the beat future against the owner deadline (LeaseKeeper::start). Dropping that future cancels the adapter sleep. One request can remain on the wire. The protocol already accepts a late beat. The pauses do not postpone the fence.

A create cannot apply twice while If-None-Match is atomic. An if-match cannot apply twice against a changed ETag. Content-hash ETag reuse on S3 is the ABA case the RFC accepts when it settles.

DST injects faults above this adapter. The in-memory codec is not S3 or Azure and has no scope, so a conflict returns on the first answer and adds no GET. I did not run DST. The cost golden is unchanged on inspection.

GQT cannot express this. The behavior is a storage answer, not a query row. The omnigraph-storage tests are the right owner.

Open PRs 1175, 1168, 1171, 1178, and 818 do not add another control-object re-issue. PR 1177 (served recovery) should call this resolver when it must settle the same class of lost answer. A third copy would drift. A future Lance release can drop this loop only when the store reports whether a conditional put was sent.

Tradeoff and liability

Correctness of the lost-ack case comes first. The copied budget is a smaller diff, and it can drift from the manifest resolver. One source of truth should own the count, the pause, and the classification. The Ok(true) widening is the part that must change before merge.

The change removes a fail-closed lost acknowledgement and a key that stayed unsettled. It adds a second policy and a success result that two initializers can share. The diff is about +945/−71 lines in 7 files. The line count does not show lower liability.

Validation

Head: 2b651998b811273f9ce5e35838d32b2eb2961e2f. Merge base: da1c46368c33876d61615add0ceb6778f9ce2b58. object_store is locked at 0.14.2. Temporary edits were removed. Nothing was pushed.

Merge base, isolated worktree, temporary test, then removed:

cargo test -p omnigraph-storage --locked --lib -- lost_applied_create_is_this_call_issue_1067 -- --nocapture

FAILED. 0 passed, 1 failed, 80 filtered out. The applied create whose answer was a generic "connection reset" returned storage write_if_absent failed for 'claim'. That is the behavior before this change.

Head, the PR tests plus one temporary case, then removed:

cargo test -p omnigraph-storage --locked --lib -- issue_1067 unscoped_s3_counts_another_writers_equal_bytes -- --nocapture

ok. 24 passed, 0 failed, 63 filtered out, 0.02s. The PR lost-answer tests pass, so that applied create now returns Ok(true). The temporary case returned Ok(true) for another writer's equal bytes on unscoped S3 after one put.

Not run here: DST, GQT, cargo fmt, cargo clippy, the workspace suite, and live S3, Azure, or Azurite. The PR body reports a wider local suite. I did not re-run it. I read object_store 0.14.2 source for S3 put mode, the retry map (409 is AlreadyExists, 412 is Precondition, 403 and 401 are auth errors), and the Azure put mapping. The Lance object-store guide was read as extracted page text. The scripted tests follow error variants. They do not follow HTTP timing.

Inline comment, crates/omnigraph-storage/src/lib.rs line 1970, RIGHT, blocking:

Ok(true) here means the stored bytes equal this payload. acquire_init_claim treats that as exclusive ownership and then runs init_commit_phase.

PreparedGraphCreate::claim_payload is the same document for every caller of one token. apply_prepared_graph_create uses storage_for_uri, which is unscoped. On unscoped S3 and Azure the first AlreadyExists falls through to this read-back (conditional_requests_may_repeat), so a second apply gets Ok(true).

A temporary case at this head showed that: foreign equal bytes, S3 codec, no scope, one put, result true. The case was removed. The same arm returns Ok(true) after a lost answer on a scoped client when the other writer stored this payload. A clean conflict on a scoped client still returns Ok(false).

Keep Ok(true) for a writer-unique payload whose copy landed. Return Ok(false) when a shared payload is already present. Add that case beside the issue_1067 tests. The if-match arm uses the same byte test and is sound where identical bytes are the same effect (lock records, state.json).

Inline comment, crates/omnigraph-storage/src/scoped.rs line 564, RIGHT, optional:

This treats permission and authentication errors as refused before the put could apply. The manifest classifier treats the same errors as ambiguous, because a client that retries can apply on an earlier attempt and then surface 403 or 401.

That matches a scoped client (max_retries is 0). It does not match an unscoped S3 or Azure client, whose default max_retries is 10. A lost 5xx that did apply, followed by an auth error, returns here with no read-back. The next same-payload call still recovers through AlreadyExists. Please share one classification with the manifest resolver, and keep the single-attempt client as the explicit exception.

…h-create claim

The adapter reads a lost create's answer back by the stored bytes, so equal
bytes must identify the writer. A prepared graph-create claim was a digest
of the token, the same bytes for every apply of it, so two applies could
both read the claim as their own. Each apply's claim now carries a fresh
attempt id (version 3). Settlement recognizes the token's claim by digest,
graph commit id and root, and still accepts the version-2 claim earlier
releases wrote.

omnigraph_storage::conditional owns the re-issue count, the pauses, the
failure classification and which answer ends a request, for control-object
writes and the manifest commit handler alike. An authorization failure is a
refusal only on a single-attempt client; an unscoped S3 or Azure client
repeats requests, so its 403 or 401 is re-issued and read back.

A manifest re-issue that meets a held version is now unresolved rather than
held: on S3 the conflict can answer the earlier copy still in flight, so a
missing version on Lance's read-back must not let it delete the transaction
file that copy names.

ragnorc commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Recommendation: ready.

Why

A lost answer on a conditional control-object write failed on the first error. The review of 2b651998 found a second break. Equal stored bytes were treated as this call's create. The prepared graph-create claim was the same document for every apply of one token. Two applies could both take the init claim.

What

Commit 94691fae shares one re-issue policy for control-object writes and for manifest commits. It also names each apply of a prepared graph-create token. Settlement still recognizes every apply of that token, and the older claim that names no apply.

How

omnigraph_storage::conditional owns the re-issue count, the pauses, and which failures are lost answers. Control-object writes and the manifest commit handler both use it.

A single-attempt client treats the first conflict as final. It also treats an authorization failure as a refusal. A client that retries inside object_store does neither. A local control-object put finishes before it answers, so a local error is a refusal there.

PreparedGraphCreate::claim_payload writes claim version 3 with a fresh attempt id. write_text_if_absent still returns Ok(true) when the stored bytes equal this payload. Two applies write different payloads. The second apply gets Ok(false), and acquire_init_claim returns InitializationClaimed. holds_claim matches the digest, the graph commit id, and the root. It accepts any non-empty attempt id on version 3. It also accepts a version 2 claim with no attempt id.

A manifest re-issue that then meets a conflict is recorded as unresolved. Lance 13 verify_commit_outcome treats a missing version as absent only when is_version_not_found_definitive is true. An unresolved request makes that flag false, so Lance keeps the transaction file and reports the commit status unknown. The first conflict, with no earlier lost answer, stays a held version. Lance reads that version back by its transaction.

Impact

A lost acknowledgement of a writer-unique payload can resolve as this call. A second apply of one prepared token stays excluded, including when the client reads a create conflict back by the stored bytes. A manifest commit whose re-issue meets a conflict stays unknown until a read shows the version.

Prior findings

Blocking. Addressed. The adapter still returns Ok(true) for equal bytes. That result holds exclusive ownership only when the payload names the writer. The prepared claim now names the apply. The unprepared init claim, the lock record, and the bootstrap lock already carry a unique id. A lease beat is stored under that lock id. state.json and a content-addressed resource are the same effect when the bytes match.

Optional 1. Addressed. One policy owns the count, the pauses, and the failure classes. The pauses are 0.2 s, 0.4 s, 0.8 s, and 1.6 s. Authorization failures are refusals only on a single-attempt client. The manifest handler cannot see whether Lance's client retries, so it keeps the repeating-client rule. That was already the manifest rule.

Optional 2. Addressed on the user page. A direct command's client retries up to max_retries, 10 by default, and an S3 if-match also retries HTTP 409. Those attempts are not in the re-issue log. A server scope sets max_retries to 0. In object_store 0.14.2 the retry budget is exhausted when max_retries is 0, so that inner 409 retry does not run.

Optional 3. Addressed. the_client_decides_what_a_failed_copy_says_issue_1067 shows three store facts. A local error is not re-issued. An unscoped first 409 over an absent object stays unresolved. Equal bytes return Ok(true).

New findings

No blocking findings. No optional findings.

Tradeoff and liability

The lost-ack fix has to return success when this call's bytes are already stored. A rule that every existing object is Ok(false) would reject that call. The exclusive-claim fix belongs on the payload.

The change removes a second copy of the re-issue budget and a claim that every apply of one token shared. It adds one policy module, one attempt-id field, and the parser settlement uses. Five more conditional writes should call this policy. A new exclusive create must put a fresh id in its bytes. The trait text on write_text_if_absent states that rule. A wrapper type would not make the store's byte compare enforce it.

Two differences stay, and each has an owner. A local control-object error is a refusal, because that put finishes before it answers. A local manifest error stays ambiguous, because Lance's Windows rename can create the manifest and then fail. The manifest handler also settles the first conflict, because Lance reads the version back.

Other open work

No other open pull request adds a second re-issue loop. PR 1177 creates a maintenance journal with write_text_if_absent when the journal has no version yet. That call uses this resolver. The journal bytes must be the effect, or they must name the writer. A shared payload used as exclusive ownership would reopen this claim bug. PR 1181 adds lease-beat log lines to the same user page. A merge should keep those lines and the re-issue log lines from this pull request. A future Lance release can drop the re-issue only when the store reports whether the conditional put was sent.

Validation

Head 94691faec1e5b92b9d65d0f962ac071139ce6f1b. Merge base da1c46368c33876d61615add0ceb6778f9ce2b58. Previous reviewed head 2b651998b811273f9ce5e35838d32b2eb2961e2f. object_store is locked at 0.14.2. Lance is locked at 13.0.0. Temporary tests were removed. Nothing was pushed.

Head, isolated checkout:

cargo test -p omnigraph-storage --locked --lib -- issue_1067 -- --test-threads=8

ok. 26 passed, 0 failed, 64 filtered out, 0.02s.

cargo test -p omnigraph-core --locked --lib -- issue_1067 -- --test-threads=8

ok. 5 passed, 0 failed, 80 filtered out, 4.63s. A lost manifest answer followed by a conflict stays unresolved.

cargo test -p omnigraph-engine --locked --lib -- one_prepared_token_has_one_init_claim_owner_issue_1067 -- --nocapture

ok. 1 passed, 0 failed, 555 filtered out, 0.01s. A second apply of one token is InitializationClaimed when the adapter reads a create conflict back by bytes.

cargo fmt --all --check

exit 0.

Merge base, isolated checkout, temporary test, then removed:

cargo test -p omnigraph-storage --locked --lib -- lost_applied_create_is_this_call_issue_1067 -- --nocapture

FAILED. 0 passed, 1 failed, 80 filtered out. The applied create whose answer was connection reset returned storage write_if_absent failed for 'claim'. The object held owner-a. The call did not return Ok(true).

Previous reviewed head, isolated checkout, temporary test, then removed:

cargo test -p omnigraph-engine --locked --lib -- one_prepared_token_has_one_init_claim_owner_issue_1067 -- --nocapture

FAILED. 0 passed, 1 failed, 555 filtered out. Two claim_payload calls returned the same version 2 document. The digest, graph commit id, and root were equal. The document had no attempt id.

GQT cannot express this. The behavior is a storage answer and an init claim, not a query row. The omnigraph-storage tests and the engine init-claim test are the owners.

Not run here: DST, the workspace suite, cargo clippy, and live S3, Azure, or Azurite. I read object_store 0.14.2 for the S3 put, the retry map, and the Azure put mapping, and Lance 13 verify_commit_outcome.

Checked, and left as sound

The lease beat treats Ok(true) and Ok(false) as a present beat. The beat object is under that lock id. The takeover claim does not decide the next owner. The if-match on lock.json does.

A clean conflict on a scoped client returns Ok(false) for a create and Ok(None) for an if-match. An S3 if-match 409 is AlreadyExists, and this code re-issues it. An Azure if-match 412 is Precondition. On a scoped client that conflict ends without a read-back. Azurite's 409 on an if-match stays AlreadyExists, so the client re-issues it and then reads the object back.

@ragnorc

ragnorc commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. All three points are addressed in 94691fa.

Blocking: equal bytes from a shared payload. Confirmed. The prepared claim was a digest of the token, so every apply of one token wrote the same bytes. I took the second option: each apply's claim now names the apply. Version 3 carries the token digest, the graph commit id, the root and a fresh attempt_id. Settlement (settle_prepared_graph_create_after_quiescence) recognizes the token's claim by those three fields, whichever apply wrote it. It still accepts the exact version-2 claim, with no attempt id, that earlier releases wrote. The adapter keeps one rule: equal bytes are this writer's. The trait text now says why contents must name the writer.

I checked every other write_text_if_absent and write_text_if_match caller. Lock and bootstrap records carry a ULID lock_id, and the unprepared init claim an attempt_id. A lease beat is a sequence in its owner's own directory, and the successor claim carries a lock id; both callers ignore the returned bool. state.json and content-addressed resources are the same effect when the bytes match. The "{}" creates in lease.rs are test code.

Regression: db::omnigraph::tests::one_prepared_token_has_one_init_claim_owner_issue_1067. It drives the recording adapter with a create that reads an existing key back by its bytes, as an unscoped S3 or Azure adapter does. Two claims of one token leave the second InitializationClaimed. Settlement recognizes any apply's claim of the token and the legacy version-2 claim, and refuses another token's claim and the mixed shapes. Before the fix, the second claim returned Ok(true).

Optional 1: one resolver. New omnigraph_storage::conditional, used by both paths, owns:

  • the count (CONDITIONAL_REQUEST_REISSUES) and the pause (reissue_pause);
  • the failure classification (classify_failed_request(error, RequestClient));
  • the decision of which answer ends a request (ConditionalRequest::answer).

The single-attempt exception is explicit: an authorization failure is a refusal only for RequestClient::SingleAttempt. The scope's refused_before_applying is that classification on a single-attempt client. An unscoped S3 or Azure adapter is Repeating, so its 403 or 401 is re-issued and then read back. The manifest handler (lance_clone) runs on the same policy and keeps a thin wrapper over Lance errors. It treats Lance's provider as repeating, because it cannot tell it from a scope's client. Its classification is unchanged.

Moving the manifest loop surfaced the trap this PR fixed for control objects. A re-issue that met a held version was recorded Held, and Held let Lance treat a missing version as definitive and delete the transaction file. On S3 that 409 can be the earlier copy still in flight. A re-issue's conflict is now Unresolved, so Lance's read-back of an absent version proves nothing and the file stays. The first copy's conflict stays Held. A direct writer's retrying Lance client can repeat that copy itself, which the handler cannot see. lance.md records that limit. A refused re-issue is now re-issued again, for both paths, instead of ending the request at once; the manifest path already did that.

Optional 2: the user page. storage-failures.md now says the server's single-attempt clients refuse authorization errors at once. A direct writer's client retries a request itself before any re-issue: up to max_retries, 10 by default, and an S3 if-match's 409 too. Those attempts are not logged, and that client's authorization error is re-issued. The new manifest version held after a re-issue log line is listed too.

Optional 3: store facts. the_client_decides_what_a_failed_copy_says_issue_1067 covers:

  • a local generic error is not re-issued (one put, not unresolved);
  • a repeating client's permission error is re-issued, while a scope's is refused after one put and leaves the scope settled;
  • an unscoped first 409 over an absent object stays unresolved;
  • another writer's equal bytes read back as Ok(true), the contract the claim change relies on.

The Azurite 409 and real Azure 412 difference ends in the same read-back, as you note. Garage-style stores that ignore If-None-Match stay unsupported, as for manifests.

PR #1177 should resolve through omnigraph_storage::conditional rather than a third copy.

Validation at 94691fa:

  • the storage crate's unit tests: 90 passed;
  • the focused issue_1067, conditional::, manifest_request and init-race tests across storage, core, catalog and engine: 43 passed;
  • clippy -D warnings on those four packages. The local toolchain is 1.98 and the repository pins 1.97.1. Two 1.98-only lints, on history.rs and graph_coordinator.rs, are untouched here and were allowed locally.
  • cargo fmt, check-docs.py and typos.

The workspace suite runs in CI.

ragnorc commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Recommendation: ready.

Why

A lost answer on a conditional control-object write failed on the first error. The review of 2b651998 found a second break. Equal stored bytes were treated as this call's create. The prepared graph-create claim was the same document for every apply of one token.

Commit 94691fae fixed both, and the review of that commit was ready. Commit cccf1178 keeps that behavior. It registers the one shared conditional put in the durable-call inventory. At 94691fae the production adapter already had one .put_opts( call, and the inventory still expected two.

What

omnigraph_storage::conditional remains the one re-issue policy for control-object writes and for manifest commits. Each apply of a prepared graph-create token still writes its own attempt id. The guard graph_visible_write_chokepoints_are_registered now expects one .put_opts( in omnigraph-storage/src/lib.rs.

How

ObjectStorageAdapter::send_conditional_put is that one call (crates/omnigraph-storage/src/lib.rs). write_text_if_absent and write_text_if_match both use it. The inventory walks the syntax tree and skips test items, so the scripted stores do not count. The registered label names this shared primitive (crates/omnigraph/tests/forbidden_apis.rs).

A single-attempt client treats the first conflict as final. It also treats an authorization failure as a refusal. A client that retries inside object_store does neither. A local control-object error is a refusal. LocalFileSystem::new() leaves fsync off, and the create path publishes with hard_link before it returns.

PreparedGraphCreate::claim_payload writes claim version 3 with a fresh attempt id. Settlement matches the digest, the graph commit id, and the root. It accepts any non-empty attempt id on version 3, and a version 2 claim with no attempt id.

A manifest re-issue that then meets a conflict is recorded as unresolved. Lance 13 verify_commit_outcome treats a missing version as absent only when is_version_not_found_definitive is true. An unresolved request makes that flag false, so Lance keeps the transaction file. The first conflict, with no earlier lost answer, stays a held version.

Impact

A lost acknowledgement of a writer-unique payload can resolve as this call. A second apply of one prepared token stays excluded, including when the client reads a create conflict back by the stored bytes. The guard fails if a second production conditional put appears in the storage adapter without a new registration. Runtime behavior is the same as at 94691fae.

Blocking

No blocking findings.

Optional

No optional findings.

Tradeoff and liability

The lost-ack fix has to return success when this call's bytes are already stored. A rule that every existing object is Ok(false) would reject that call. Exclusive ownership belongs in the payload. The prepared claim now names the apply. The trait text on write_text_if_absent states that contents must identify the writer.

The change removes a lost acknowledgement reported as a hard failure, a key that stayed unsettled, a claim that every apply of one token shared, and a second copy of the re-issue budget. It also stops a manifest re-issue's conflict from letting Lance delete the transaction file that an earlier copy may still name.

It adds one policy module, one attempt-id field, and the parser settlement uses. The guard update is the existing inventory, pointed at the one primitive. Five more conditional writes should call this policy. A new exclusive create must put a fresh id in its bytes. A new .put_opts( in the storage adapter must be registered. A wrapper type would not make the store's byte compare enforce the writer id.

Two differences stay, and each has an owner. A local control-object error is a refusal, because that put finishes before it answers when fsync is off. A local manifest error stays ambiguous, because Lance's Windows rename can create the manifest and then fail. The manifest handler also settles the first conflict, because Lance reads that version back. A direct writer's retrying Lance client can repeat that copy itself. The handler cannot see those copies. docs/dev/lance.md records that limit. The server scope sets max_retries to 0, so that inner retry does not run there.

Other open work

No other open pull request adds a second re-issue loop. PR 1177 adds served maintenance recovery and does not add its own conditional-put policy. It also edits docs/user/operations/storage-failures.md above the log list. PR 1181 appends lease-beat log lines on that same page. A merge should keep this pull request's re-issue lines and those lease lines. A future Lance release can drop the re-issue only when the store reports whether the conditional put was sent.

Validation

Head cccf1178e0a240e2d2f8c599d9543061a371645f. Merge base da1c46368c33876d61615add0ceb6778f9ce2b58. Previous reviewed head 94691faec1e5b92b9d65d0f962ac071139ce6f1b. object_store is locked at 0.14.2. Lance is locked at 13.0.0. The temporary test was removed. Nothing was pushed.

GQT cannot express this. The behavior is a storage answer and an init claim, not a query row. The omnigraph-storage tests and the engine init-claim test are the owners.

Head, isolated checkout, CARGO_TARGET_DIR=/workspace/target:

cargo test --locked --lib -p omnigraph-storage -p omnigraph-core -p omnigraph-engine -- issue_1067 -- --test-threads=8

ok.

  • omnigraph-core: 5 passed, 0 failed, 80 filtered out, 4.64s. A lost manifest answer followed by a conflict stays unresolved.
  • omnigraph-engine: 1 passed, 0 failed, 555 filtered out, 0.01s. A second apply of one token is InitializationClaimed when the adapter reads a create conflict back by bytes.
  • omnigraph-storage: 26 passed, 0 failed, 64 filtered out, 0.02s.

cargo test --locked -p omnigraph-engine --test forbidden_apis graph_visible_write_chokepoints_are_registered -- --exact --test-threads=8

ok. 1 passed, 0 failed, 35 filtered out, 1.13s.

Merge base, isolated checkout, temporary test, then removed:

cargo test --locked -p omnigraph-storage --lib -- lost_applied_create_is_this_call_issue_1067 -- --nocapture --test-threads=8

FAILED. 0 passed, 1 failed, 80 filtered out. The create applied. The object held owner-a. The call returned storage write_if_absent failed for 'claim': Generic S3 error: connection reset. It did not return Ok(true).

Exact-commit CI on cccf1178 passed, including Test Workspace, Lint (clippy), Format, the DST pinned suite, GQT, RustFS, and Azurite (run 40480168835 and the matching DST and GQT runs). I did not re-run those suites locally. I did not run live S3 or live Azure. I read object_store 0.14.2 for the S3 put, the retry map, and the Azure put mapping, Lance 13 ConditionalPutCommitHandler and verify_commit_outcome, and the Lance object-store and transaction pages.

Checked, and left as sound

The lease beat treats Ok(true) and Ok(false) as a present beat. The beat object is under that lock id. The takeover claim does not decide the next owner. The if-match on lock.json does.

A clean conflict on a scoped client returns Ok(false) for a create and Ok(None) for an if-match. An S3 if-match 409 is AlreadyExists, and this code re-issues it. An Azure if-match 412 is Precondition. On a scoped client that conflict ends without a read-back. Azurite's 409 on an if-match stays AlreadyExists, so the client re-issues it and then reads the object back.

A re-issue's 409 does not clear an unsettled key. Mutation::finish marks that guard settled, and the earlier unsettled entry stays until success or a read-back shows the precondition failed. That matches the scope comment.

ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 28, 2026
Record the intent nonce and resolve by commit_id_answers; keep an
in-doubt publication non-terminal; release the writer gates before the
artifact scan; add a resume position, resource ceilings and per-table
failure isolation; require maintain on every route; depend on ModernRelay#1175
and ModernRelay#1176.
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 29, 2026
* rfc: remote optimize through the serving owner

A bounded, manually requested optimize of a served graph's main branch,
executed by the process that serves and publishes it: today's gate shape
and one publication, a gate-hold bound with an exact outcome per table, a
durable operation record under a caller-chosen id before any effect,
lookup by that id after a lost response, and restart resolution from the
graph's history. Replaces the closed served-maintenance proposal in the
form its review asked for.

* rfc: link the remote optimize RFC to its discussion

* rfc: revise remote optimize after review

Record the intent nonce and resolve by commit_id_answers; keep an
in-doubt publication non-terminal; release the writer gates before the
artifact scan; add a resume position, resource ceilings and per-table
failure isolation; require maintain on every route; depend on ModernRelay#1175
and ModernRelay#1176.

* rfc: settle remote optimize resume, ceilings and resolution after second review

* rfc: settle remote optimize resolution reads and the artifact bound after third review

* rfc: resolve through the buffered commit lookup and count both offset sides after fourth review

* rfc: state the history cost of a released commit and reuse the collector's path resolution after fifth review
@ragnorc
ragnorc added this pull request to the merge queue Oct 29, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant