design: validate durable delivery of human phrase decisions - #841
Conversation
…ion handler Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
|
The content here is decision material — it states a policy and asks maintainers to approve it — and the thinking in it is good. My objection is only about where it lives.
Please give this its own numbered record in #832 in this same batch is the pattern to copy: it moved its reasoning into One thing that is not my call and that I have flagged for the maintainer: whether the executable prototype itself belongs in the repository. Separately, and this one does block: The |
|
Decision on the committed prototype directories, so all three of these can move at once: the conclusions belong in a numbered record; the harness stays out of the repository. The test we are applying is not "prototype versus production". It is: after the decision is made, will anyone run this again?
There is a second reason, and it is the stronger one: an executable specification that is not the implementation is a second source of truth. This is the thing the codebase avoids everywhere else. 0032 refuses to store an expression as a string and evaluate it at run time; the rule model is structured precisely so that what the page shows is generated from what runs and cannot drift from it. A policy module that encodes the rules beside the validator rather than inside it is that same drift, with the added property that nothing will ever fail when the two disagree. So: please lift the reasoning and the decisions requested into a numbered record under None of this is a judgement on the work itself. The analysis is good and the experiments clearly did their job; the argument is only about what survives them. Applied to this PR, with one carve-out:
|
…prototype Signed-off-by: dada-yan <BinjunYann@gmail.com>
…izer. Signed-off-by: dada-yan <BinjunYann@gmail.com>
|
This is still marked as a draft, so I have not merged it — a draft is your signal that it is not finished, and flipping that is your call rather than mine. From my side the rework answers everything I raised: the reasoning is now a numbered record with rows in both index tables, the accepted record carries a dated revision note pointing at it, and the committed harness is gone. CI is green on all four checks and it merges cleanly onto Mark it ready for review when you consider it done and I will merge it. |
Signed-off-by: dada-yan <BinjunYann@gmail.com>
# Conflicts: # docs/decisions/README.md
) (#864) * Bound lock waits for human phrase alignment review decisions (#798, #800) Survey of every synchronous handler that takes a lock a background job can hold (asked in the second sentence of #798): - decide_alignment_kind_word — fixed in #828 by decide_and_apply_human. Closed. - decide_alignment_phrase — calls materialize inline at review_routes.rs:1280. materialize takes pg_advisory_xact_lock on 'typed_materialize' keyed by kb_id; align_phrases worker holds the same lock. Without a budget, a review click can pin a connection for minutes on a large base (#800 measured 'first run 4.2545363s' at 500 statements, observed a third-acquire pool timeout on a two-connection pool). #800 was closed as completed against design #841; the code fix did not land. This PR lands it. - decide (generic review) — calls merge_entities → temporal.rs per-timeline advisory locks. Real but more invasive (multiple lock sites). Deferred to a follow-up; this PR is the minimum cut. - decide_mapping, decide_defect, decide_violation, decide_pending, decide_proposal, apply_import — single-row UPDATE or no relevant lock; lower priority, deferred. This change: - Add materialize_human (utopia-store): same body as materialize, but sets SET LOCAL lock_timeout = '2s' at the top of the transaction and maps PostgreSQL 55P03 to AppError::CodedConflict { code: 'alignment_busy' } with a 'please try again' message. Worker path (materialize) is untouched — workers don't have a spinner. - Switch decide_alignment_phrase to call materialize_human. Tests in crates/utopia-store/tests/human_materialization_has_a_lock_budget.rs: - With the typed_materialize lock held by another connection, materialize_human returns CodedConflict("alignment_busy") well within 10 seconds, with a retry message. Worker materialize completes normally once the lock is released. - With no contention, materialize_human produces the same Outcome as materialize. SHOW lock_timeout returns empty after the call, confirming SET LOCAL did not leak to the session. Signed-off-by: rollroyces <royce@rollroyces.com> * test: lock_timeout leak assertion doesn't assume empty default The Postgres session-level default for lock_timeout is '0' (wait forever), not the empty string. SHOW lock_timeout returns Some('0') on a fresh session — not Some(''). Comparing to '' failed in CI. Capture the value before and after a second materialize_human call, and assert they're equal. This pins the SET LOCAL to the transaction it opened without depending on what the surrounding session happens to default to. Signed-off-by: rollroyces <royce@rollroyces.com> --------- Signed-off-by: rollroyces <royce@rollroyces.com> Co-authored-by: rollroyces <royce@rollroyces.com> Co-authored-by: WaylandYang <145302500+WaylandYang@users.noreply.github.com>
The durable-delivery reasoning now lives in ADR 0051, indexed in both tables, with a dated contextual note in 0044. The one-off script README is removed; the real Rust regressions stay.
The unused public
try_materializeentry point is removed. Productionmaterializestill calls privatematerialize_in_tx, and productionphrase_bindings::decidecallsdecide_on. Validation still precedes pool acquisition on the existing decide path. The integration target is nowhuman_phrase_materialization_delivery; normal recomputation and process-exit cases use the existing public materializer.Busy orchestration lives only in
materialize_delivery_tests.rsundercfg(test). It acquires the try-lock, calls the same real private body after success, and uses real Deferred handling on Busy. The test observes productionmaterializeblocked in PostgreSQL by that lock, releases it, and verifies completion. The old-to-new assertion map retains rollback, pool release, same-job deferral, late arrivals, reverse/duplicate processing, actual worker startup, bounded deferral, scoped requeue, cost and all three OS process-kill windows.Linux validation at
7e49b7fd6d555727f169a77d36ab12367bf97003against dev69560db5969fae8def8c4e84b7572d841e1dca57:cargo fmt --all --check,cargo clippy --locked --workspace --all-targets -- -D warnings,cargo test --locked --workspace -- --test-threads=1, andcargo build --locked --workspacepassed; database and PDF requirements enabled. Workspace: 959 passed, 0 failed, 5 ignored (existing live HTTPS plus the opt-in entries below).Opt-in commands are in CONTRIBUTING and the test module headers. No production job kind, HTTP 202/status read, Review UI or multi-instance recovery contract is introduced. Partial-projection process kill and failed ack persistence remain future production acceptance; this does not close #800. The proposed contract remains Draft.
Refs #800