Repository navigation
fix(server): settle an embedded error's facts before it is logged - #1123
Conversation
The engine-failure warning in `from_omni` reads the error's status once its request's facts are on it. The embedded helpers set their own facts after it returned: `engine_error_output` judges one step, and `engine_error_output_after` takes the caller's write fact. A query execution failure judged as a step therefore warned 400 while its body answered 500 `outcome_unknown`. `from_omni_settled` takes the facts before the warning: the served path passes the request's, and each embedded helper passes its own, so the warning and the body agree on both paths.
WhyThe warning What
HowFor a normal engine error, both readers use the facts the caller passed. ImpactA Recommendation: do not merge this head. Two blocking items are open. Blocking1. Four returns skip
|
| Commit | Helper | code | effects | action | HTTP |
|---|---|---|---|---|---|
parent 10f5517c |
engine_error_output and engine_error_output_after(_, true) |
outcome_unknown |
unknown |
reconcile |
500 |
head 674433c |
same two helpers | internal |
none |
refresh |
500 |
A valid key node:Person stays conflict, effects none, action refresh, HTTP 409 on both commits. The served path is unchanged. This return already skipped with_request_facts, and into_response still applies the request fact.
An embedded write uses this disposition (EngineWriteError in command_outcome.rs). Effects unknown exits 2. Effects none exits 1. The JSON code changes from outcome_unknown to internal.
The PR text says that no body, status, or disposition changes. These exits change the embedded code and disposition. The HTTP number stays 500.
Use one exit for every return. Call settle, then emit the existing warning when completion_uncertain is set. Do not apply the original error's completion evidence to this replacement internal error. These four errors are silent today. The one exit logs them, at the settled status. Extend an_uncertain_engine_failure_logs_the_status_its_response_sends with one invalid type key through engine_error_output and through engine_error_output_after(_, true). Assert code, disposition, and the warning status.
Inline on crates/omnigraph-server/src/lib.rs:1445 (from_omni_settled): same finding. The four returns are lines 1470, 1509, 1518, and 1592.
2. Rebase onto current main and keep scoped_unknown before settle
This head conflicts with main at 888223b7. A merge of main into this commit conflicts only in crates/omnigraph-server/src/lib.rs. main (#1096) assigns response.scoped_unknown and then calls with_request_facts. This head calls settle in that same place. The warning's scoped field merged from main.
Keep both, in this order:
response.scoped_unknown = response.completion_uncertain && storage_outcome;
let response = settle(response);Keep scoped = response.scoped_unknown on the warning. docs/user/operations/storage-failures.md on main says this line records whether the uncertainty is scoped to the storage keys. A resolution that keeps only settle leaves scoped_unknown false. contains_process then treats every uncertain error as process containment, and the log field is wrong.
#1124 adds a publication field on this same warning. Capture the id before into_completion_evidence, and log it after settle.
Inline on crates/omnigraph-server/src/lib.rs:1756 (let response = settle(response)): same rebase requirement.
Optional
-
The new cases call the helpers with no request write in progress. That is the mismatch reproduced for
step(warning 400, body 500). The other direction is a read helper aftersubmithas set the request write fact: the old warning can say 500 while the body stays 400.settleoverwritesowned_writebefore the read, so this head covers that direction. One case insidescope_request, after a submitted write, would lock it. -
fix(server): send WWW-Authenticate on every 401 #1122 also edits
ApiError::into_response. This PR does not. fix(server): send WWW-Authenticate on every 401 #1122 is behind fix(server): log the status an error response sends #1117: its patch still builds the body withinto_output()and then reads the status. On currentmain,into_responsecallswith_request_factsand thenstatus(). Keep that order when fix(server): send WWW-Authenticate on every 401 #1122 rebases. TheWWW-Authenticateextension is independent ofsettle.
Tradeoff and liability
The observable contract is one status. The warning and the body are two readers. The write fact selects 400 or 500 for an uncertain DataFusion error. The warning stays at the mapping site so it shares the write task's span with the containment log. A log inside into_judged_output would run later, on the waiter. The settle callback buys that span. It is the right single door for the caller's facts.
This removes the disagreement for every error that reaches settle. The merge source-deletion path calls from_omni and then into_step_output inside the owned write. The warning runs only when completion is uncertain, and the step fact then sets owned_write, which that write already set. MCP applies after_owned_write to an ApiError the handler already mapped inside that same scope. Those two agree today.
This adds a second exit. The four early returns skip the door, and they change the embedded body. The next early return does the same. One exit removes that liability. scoped from #1096 and publication from #1124 belong on that exit. A second logger would split the contract again.
The callback is a small primitive. Five more embedded fact patterns can pass a closure into the same exit. They do not need a new type.
No hot-path cost. settle runs once per mapped error.
Validation
GQT cannot express this contract. The contract is a server tracing field against the error body. It is not rows, counts, column types, or a query error. The owner is api_error_tests in omnigraph-server. The PR extends the right test.
Before: parent 10f5517c5d41c433005603b0c5a29358ec9560a8. The three embedded assertions from this PR were added to an_uncertain_engine_failure_logs_the_status_its_response_sends. The helpers stayed as they are on the parent.
cargo test -p omnigraph-server --lib --locked api_error_tests::an_uncertain_engine_failure_logs_the_status_its_response_sends -- --exact --test-threads=1 --nocapture
Result: fail. Served read logged status=400 Bad Request. Served write, before the owned write and inside it, logged status=500 Internal Server Error, and the containment line logged 500. The embedded step case then failed. The body check passed at HTTP 500. The warning was status=400 Bad Request. The loop did not reach after a write or read.
The invalid-key probe on that same parent is the table above.
After: exact head 674433c7d2b35b0472e6ce2a9cd2eaadabcac78c. Same command, the PR's own assertions.
Result: pass. 1 passed.
The invalid-key probe on that head is the table above.
The temporary probe and the parent-only assertions were removed. Nothing was pushed.
CI on this exact commit passed, including Test Workspace, Lint (clippy), Format (rustfmt), and GQ Logic Tests. That run uses this head's base. The base does not contain #1096. The merge with main was not compiled.
Limitations: the full workspace suite was not run locally. Performance was not measured. This path does not depend on Lance. The log contract in docs/user/operations/storage-failures.md was read on the parent and on current main.
# Conflicts: # crates/omnigraph-server/src/lib.rs
Four translations returned an internal error before `settle` when a conflict named invalid graph type metadata, so the embedded helpers' facts never reached it: a key conflict on `Person` judged as a step answered `internal` with no effects, where it had answered `outcome_unknown`. Those returns also skipped the warning. The translation is now its own function, and `from_omni_settled` is its one exit: every result, the internal replacement included, takes its operation's facts and is logged at the status its body sends. The replacement takes no completion evidence from its cause, and main's `scoped_unknown` is set before the facts.
|
Addressed at fcd792d, after merging main (5fdf910). Blocking 1: four returns skipped Blocking 2: main. Merged; Optional 1. Added: Optional 2. Noted for #1122's rebase; this PR does not touch
|
1 participant
What
The review of #1117 left one optional note: the embedded helpers
engine_error_outputandengine_error_output_afterset their facts (one step's judgment, or the caller's write fact) afterApiError::from_omnireturned, so its engine-failure warning read the status before them. ADataFusionfailure judged as a step warnedstatus=400 Bad Requestwhile its body answered 500outcome_unknown.from_omninow delegates tofrom_omni_settled, which takes the error's final facts before the warning reads its status. The served path passes the request's facts, as before; each embedded helper passes its own.into_step_outputkeeps its behavior through the extractedinto_step. The translation is its own function andfrom_omni_settledits one exit, so every result, including the internal error a conflict naming invalid graph type metadata becomes, takes its operation's facts and is logged; that replacement takes no completion evidence from its cause. No body, status or disposition changes againstmain. Two log changes: the embedded warning now matches its body, and those four internal replacements, silent before, now log the warning. The CLI installs no tracing subscriber today, so no CLI user sees the line, but the property now holds on both paths.Why
Follow-up to the optional finding on #1117: a log line and its response must not disagree, whichever path builds them.
Tests
api_error_tests::an_uncertain_engine_failure_logs_the_status_its_response_sendsgains three embedded cases, a step (engine_error_output), a failure after a write (engine_error_output_after(_, true)) and a read (engine_read_error_output), each asserting that the captured warning's status equals the body'shttp_status().mainform and the test kept: the test fails; the warning logsstatus=400 Bad Requestfor the step case whose body is 500.cargo test -p omnigraph-server --lib --locked214 passed, 2 ignored.cargo clippy -p omnigraph-server --all-targets --locked -- -D warnings -W clippy::dbg_macro: clean.cargo fmt --all --check: clean.