client: detach, attach, and finish a transaction by bare id (#13) - #45
Merged
Conversation
A transaction can now outlive its handle. Transaction::detach stops the keep-alive thread — joined, so no ping is in flight when it returns — and leaves the transaction running; Client::attach_transaction turns the id back into a real handle, reading the ping interval from #<id>/@timeout; ping_transaction, commit_transaction and abort_transaction take the bare id, so a process holding nothing else can finish someone else's transaction. Commit rides under a mutation id (not idempotent), abort is retried on the cluster's own forgiveness, ping doubles as the liveness probe. Drop follows the C++ destructor's line: a handle this process started still aborts on drop — unchanged, and what keeps ? safe inside a transaction — where an attached one detaches, because an attacher's failure must not destroy the owner's work. attach_transaction always pings; Go's AutoPingable:false maps onto with_transaction plus the by-id commands. A failed attach names the operation and the id, because the cluster's own answer does not always do either: a garbage id earns 'Unknown cell tag 0' with no id in it, and an expired one 'No such object', both observed on a local cluster. tests/transaction_lifecycle.rs pins the lifecycle at the wire level from an in-process stub — detach sends no abort, attached drop sends no abort, started drop still aborts exactly once, the by-id triple carries the id it was given, and a detach racing its own in-flight ping neither panics nor aborts. The detach example runs the handoff against a cluster and checks itself; the whole flow was verified on a local cluster, including a commit from a second client and by-id pings keeping a 3 s transaction alive for 7.
The README is the record of which cluster examples have actually been run; detach ran green against a local cluster — the handoff, the attached drop, the started drop, and the by-id finish.
…s that fail without it Three findings from review, and the test defects behind them. An attached handle could lose the transaction it was handed, silently. `attach` read `#<id>/@timeout` — the *configured* lifetime — and learned nothing about the remaining one, while the keep-alive's first ping was a whole interval away. At the default 30 s, a handoff completed at t=21 s scheduled its first ping for t=31 s and the cluster expired the transaction at t=30 s; the ping earned `No such transaction`, the thread returned, and the handle went on looking healthy. Any handoff slower than `timeout × 2/3` lost the transaction that way. `attach` now pings before it returns: the clock restarts at the attach, and a transaction that died in the handoff is this call's error rather than a later command's. `Transaction::is_lost` makes the other half visible — the keep-alive stops for exactly one reason, and that verdict used to be invisible to the handle's owner. `detach` promised a wait bounded by half the ping interval; the real bound was the ping's own request budget, `min(interval / 2, 120 s)` — two minutes for the hour-long launcher transaction the code reasons about. The join is now genuinely bounded at five seconds, on an `mpsc::recv_timeout` against a channel the ping thread's own `Sender` closes, since `std` has no timed join. The doc also stops overclaiming: `detach` sends nothing itself, but the keep-alive can get one last ping away before it sees the stop, and that ping is waited for — for the five seconds, and no further. Past them it is left in flight and `detach` returns anyway, which can only happen above a 30 s timeout: below that the ping's whole budget fits inside the bound, and at the default the two are equal. Nothing protected the join. The race test recorded each request on arrival, so the in-flight ping was already counted before the detach and the thread never *started* another — both its assertions held with `stop` in place of `stop_and_join`, and the whole suite passed. It now asserts the wall clock: with a ping answered 500 ms late, `detach` must not return in the 8 µs it takes without the join. The unit test beside it does the same against a socket that accepts and holds. Test defects, all mutation-checked: - The by-id retry class was unasserted in both directions; `ping` and `abort` now assert the *absence* of a mutation id and its `retry` flag, which `abort_by_id: Freely -> WithMutationId` now fails. - `an_attached_handle_dropped_mid_work_sends_nothing` asserted nothing at all. Replaced by a pair against connection-counting sockets: a started handle's drop must reach the cluster, an attached one must not. `doomed()` pointed at a dead port, where an abort sent and an abort not sent look identical; `detach_hands_back_the_id_and_disarms_drop` is on a counted socket for the same reason. - The false comment about a libtest per-test timeout is gone — there is no such thing, which is part of why the join is bounded. - New wire tests for an attached handle's explicit `abort`, for `detach` on an attached handle, for `is_lost`, and for an attach whose ping is refused. The post-drop window in `attach_reads_the_timeout_...` grew from 300 ms to a second, more than one ping's request budget. Smaller things: `@timeout` is read as either `Int64` or `Uint64` and a negative, zero or non-integer is now an error naming the attribute instead of a zero that floored the interval to a 1 Hz pinger; `attach_transaction` takes `&str` like its three by-id siblings; attaching twice, and why `mem::forget` is not a way to hand a transaction on, are documented. AGENTS.md gets the branch's cluster facts — `@timeout` in milliseconds as `Int64`, and the three different errors for a garbage id, an expired object and a refused ping — and its test count. Verified against a local cluster: the five handoff scenarios, plus the late-attach one both ways — with the ping the transaction survives 20 s past a handoff that spent seven ninths of its life, and without it the attach reports success and `is_lost` goes true.
… it does `detach` claimed, in bold, that no ping is in flight when it returns. That is true only up to a 30 s timeout, and the docs never said so. The join is bounded at five seconds; a ping's own request budget is `clamp(interval / 2, 1 s, 120 s)` on an `interval` of `max(timeout / 3, 1 s)`. The budget therefore fits inside the bound while the timeout is under 30 s, equals it at the 30 s default, and exceeds it above — so for every long-running launcher transaction the module is written around, a ping stalled on a proxy that has stopped answering ends in the timeout branch, where the `JoinHandle` is dropped and the thread detached. It is not leaked: the loop re-reads the stop flag as soon as its ping ends, so at most one ping is outstanding and it exits inside that same budget. But the ping can reach the master *after* `detach` returned and restart the expiry clock there, and the transaction then lives a full timeout from wherever it landed — the one thing `detach` exists to rule out. The bolded sentence licensed killing the process on return, so it now says the bound, names the crossover, and says what happens past it; `DETACH_JOIN_TIMEOUT`'s own doc stops calling the cost "one interval of extra life", which understated it by a whole timeout. No behaviour changed: bounding the join by the ping budget instead would restore the two-minute `detach` the bound was added to remove. Nothing guarded the bound. Replacing the ping thread's `let _alive = alive` with `drop(alive)` closes the channel at once, `recv_timeout` answers `Disconnected` every time, and `stop_and_join` degrades to the unbounded `join()` the channel was built to avoid — with the whole workspace still green, because the only test of it asserts a *lower* bound. The new test asserts the upper one: a proxy that accepts and never answers, a ping budget of 30 s, and `stop_and_join` must return inside five. Under the mutation it measures 30.0006 s and fails; every other test in the workspace still passes, which is the point. `is_lost` gets its two false negatives written down. False is not "something is pinging": a thread that never started because the spawn failed reads false, and so would one that panicked — nothing on the ping path panics today, and a poisoned lock is recovered rather than unwrapped, so that half is about a future edit. Neither is visible from the handle and a ping does not expose them, since it answers for the transaction rather than for the thread. It is also `&self` where `detach` consumes the handle, so after a detach the only probe left is `ping_transaction` on the id. Three smaller things, all noted rather than paid for: - `stop_pinging` drops the keep-alive, and with it the flag `is_lost` reads. Every caller today is terminal, so it is unobservable; a future `&mut self` method calling it would silently reset a true verdict. - `start_transaction` rides the retry pipeline under a mutation id, so a start that *was* retried hands back the transaction the first attempt created and its clock started there — the same staleness `attach` pings to close, but needing a lost answer *and* retries slower than the timeout, where the handoff window was every handoff past two thirds of it. - the attach ping runs on the caller's client, so `attach_transaction` is two retryable round trips, not two quick ones. That is the right way round — a ping with a caller waiting on its verdict should not fail over one dropped packet — and now it is written down. Measured this round on a local cluster: the master honours the asked-for timeout verbatim, `#<id>/@timeout` reading back 3600000, 30000 and 20000 for transactions started at each, so the crossover is arithmetic a caller can do from what it asked for.
…etach # Conflicts: # AGENTS.md # crates/ytsaurus-client/CHANGELOG.md # docs/sdk-comparison.md
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
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.
Closes #13.
What
A transaction can now outlive the handle that started it, and a process holding only an id is a real participant rather than a binding.
Transaction::detach(self) -> String— disarmsDrop, stops the keep-alive and waits for it, returns the id. Nothing is sent to the cluster.Client::attach_transaction(id)— a realTransaction(bound client, keep-alive, working commit/abort/ping) withOrigin::Attached. It reads#<id>/@timeoutfor the ping interval, and that round trip doubles as existence validation.Client, so a process that has only the id string can finish someone else's transaction.commitcarries a mutation id (it is not idempotent);pingandabortcarry none (abort is forgiving on the cluster's own account).Dropfor an attached handle detaches, copying the C++ destructor: an attacher's?must not destroy work the owner still holds a handle to. Drop for a started handle still aborts, unchanged — that is what makes?safe inside a transaction, and the existing tests still prove it.Transaction::is_lost()— the keep-alive raises a flag before giving up, so a handle that has stopped pinging can be seen to have stopped rather than discovered later on an unrelated command.The bug found while building it
attachused to read@timeoutand then wait a full ping interval before its first ping.@timeoutis the configured timeout and says nothing about remaining life, so any handoff taking longer thantimeout × 2/3lost the transaction — silently, because the keep-alive's exit was quiet. Demonstrated live, both ways, on a 9 s transaction handed over after 7 s:attachnow pings before it returns, so the clock restarts at the attach and a transaction that died during the handoff becomes this call's error.Where the promise stops, stated rather than hidden
detachwaits for the keep-alive through anmpsc::recv_timeouthandshake — the timed joinstdlacks — bounded at five seconds. That bound is reachable, and the docs now say so instead of promising more than it delivers:min(interval/2, 120 s)and the interval istimeout/3, so five seconds covers the whole budget only while the timeout is at or under the 30 s default;detachreturns with a stalled ping still in flight; that ping can reach the master afterwards and restart the expiry clock, so the transaction lives a full timeout from wherever it landed;Bounding the join by the ping budget instead was considered and rejected in the commit message: it would make
detachblock up to two minutes on exactly the hour-long launcher transaction this module is written around.ureqoffers no way to cancel another thread's in-flight request, so there is no third option — the honest sentence is the deliverable.Acceptance criteria → tests
a_started_handles_drop_still_abortsandonly_a_started_handles_drop_reaches_for_the_cluster, the latter contrast-based on a connection-counting socket.a_detached_transaction_is_neither_aborted_nor_pinged_again(1.6 s of silence against a 1 s interval).detach_with_a_ping_in_flight_waits_for_itasserts wall clock ≥ 250 ms against a stub answering 500 ms late. Under the non-waiting mutationdetachreturns in 8 µs and the test fails. The earlier version of this test passed either way, because the stub recorded requests on arrival and both assertions only asked whether a new request had started.stop_and_join_gives_up_on_a_ping_that_outlasts_the_bound, against a proxy that accepts and never answers with a 30 s request budget. Degrading the bounded join back to an unbounded one gives 30.0006 s and fails; it is the only test in the workspace that catches that.mutation_idasserted absent on ping and abort, so demoting abort's retry class fails.Validation
Four independent passes — a critic, a test validator, and two verification rounds. The verifier measured rather than reasoned: the panic path (
recv_timeoutdisconnects in 2.5 µs,joinreturnsErrin 18.5 µs, discarded), the post-disconnect join window (worst of 200 idle runs 384 µs; worst of 50 after a real ping 2.2 ms), and the timing tests' safety — they assert lower bounds on a stub-controlled delay, so CPU load pushes the measurement away from the threshold, structurally the opposite of thefix/test-port-racescar. The one upper-bound test overshoots its 5 s bound by 0.3–6.0 ms at load average 71, against 2 s of headroom.Flakiness: 12/12 clean, six of them at load average 91 on ten cores. Gates:
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(652 passed),cargo test --workspace --doc(64 passed). Newexamples/detach.rsruns green end to end.Follow-up candidates (out of scope)
detachremains best-effort about the in-flight ping. Now stated, not fixed; the rejected alternative is written down should anyone revisit it.is_losthas two false negatives — a panicked keep-alive thread and a failed spawn. Nothing on the ping path panics today; both are documented, and a tri-state was rejected because the remedy for either is the same (ping, or attach afresh).start_transactionunder retry has the same stale-clock exposureattachnow closes, but needs a lost answer and retries slower than the timeout.Tx.Finished()-style watcher remain unbuilt;is_lostis polled.