Add F3 payload layer: receiver-confirmed completion, receiver budgets, awaitable transfer facade#4865
Conversation
…ozen outcomes Four review findings on the F3-1 outcome-recording path: 1. Register the incarnation BEFORE the tx is monitor-visible. new_transaction() inserted into _tx_table first; a monitor tick in the window could terminate the tx, its outcome dropped by the incarnation guard (terminated-but-unknown gap), and the subsequent registration left a dead _tx_incarnations entry that nothing ever pops. Registration now precedes table insertion. 2. Retire a still-live prior transaction on tx_id reuse. A plain _tx_table overwrite orphaned the old tx: its refs stayed servable in _ref_table forever (the monitor only discovers transactions via _tx_table) and its sources were never released. new_transaction() now delete+transaction_done()s the old tx; its outcome is dropped by the incarnation guard -- the retry is authoritative. 3. Make tx required in _record_outcome() so no call site can opt out of the incarnation guard. With that, the _accept_outcomes flag is provably redundant (shutdown clears all incarnations under the same lock), so it is removed -- one mechanism, one story: outcomes record only for live registered incarnations. 4. Deep-freeze TransferOutcome/RefOutcome: frozen=True only blocks attribute rebinding; refs becomes a tuple and receiver_statuses a MappingProxyType over a private copy, so outcome_cb consumers and pollers cannot mutate the recorded per-receiver truth. Also guard downloaded_to_one/downloaded_to_all with _invoke_cb_safely: a raising user callback no longer loses the EOF reply or permanently skips downloaded_to_all. Tests: ordering regression (incarnation-before-visibility), active-reuse retirement, deep-frozen outcome, raising-callback serving path; _add_tx and the shutdown test updated for the required-tx signature.
…s, status Address design-review findings: - Forward-path heartbeat rule narrowed: the no-expiry exemption now covers only the payload-materialization phase (TASK_ACCEPTED -> new TASK_PAYLOAD_READY message), bounded by a materialization deadline; heartbeats stay authoritative while user code trains, so a wedged process is detected at heartbeat timeout, not task timeout. Diagrams updated (mermaid re-validated). - launch_once=False moves to launch-scoped tokens: the CJ regenerates the token in each per-launch bootstrap config and stopping a process invalidates its token, so a surviving stale process cannot authenticate against a later launch. - Plan dependencies now enforce the design's payload-safety prerequisites: F3-4 hard-depends on F3-2 + F3-3 (receiver-confirmed, retry-aware outcomes; budget-bounded resolution); critical path updated (F3-2/F3-3 parallel behind F3-1, one M added, still inside the 10-14 week floor). - Explicit mapping between lifecycle transfer states and the F3 TransferOutcome vocabulary (TRANSFER_COMPLETE <=> COMPLETED; TRANSFER_FAILED <=> FAILED or ABORTED; receiver truth applies before the mapping). - Bookkeeping: PR-0 recorded as landing inside the F3-1 PR (NVIDIA#4853); EX-5 moved to Wave 4 (its EP-4 dependency) and assigned P1; CT-5 assigned P1; tier accounting line added (36 PRs, CT-4 halves in P1/P2); P0+P1 ~36 engineer-weeks. - ClientAPIBackendSpec classified as internal (frozen for parallel development, not public API in 2.9). Open Questions annotated with their deciding PRs. - Status set to Approved; Revision 2.2 entry added. - Add Topic.TASK_PAYLOAD_READY to the frozen protocol vocabulary (follow-up to NVIDIA#4856, which merged before the phase-boundary message was introduced).
There was a problem hiding this comment.
Pull request overview
This PR hardens F3 DownloadService terminal outcome recording and lifecycle handling, focusing on concurrency-safe registration/retirement of transactions, making outcome recording unskippable, and ensuring recorded outcomes are immutable when shared with pollers and callbacks.
Changes:
- Register transaction incarnations before the transaction becomes monitor-visible, and retire still-live prior transactions when a
tx_idis reused. - Remove
_accept_outcomesand requiretxfor_record_outcome()so the live-incarnation guard is always enforced. - Deep-freeze recorded outcomes (tuple refs + mapping-proxy receiver statuses) and guard
downloaded_to_one/downloaded_to_allcallbacks against exceptions; add targeted unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
nvflare/fuel/f3/streaming/download_service.py |
Adjusts tx creation/retirement sequencing, enforces incarnation-guarded outcome recording, and wraps serving-path callbacks with exception safety. |
nvflare/fuel/f3/streaming/transfer_outcome.py |
Deep-freezes outcome containers to prevent mutation of recorded terminal truth by consumers. |
tests/unit_test/fuel/f3/streaming/transfer_outcome_test.py |
Adds tests for deep-freezing, tx_id reuse retirement behavior, and updated _record_outcome(tx=...) requirement. |
tests/unit_test/fuel/f3/streaming/download_test_utils.py |
Updates isolated test service state to match production removal of _accept_outcomes. |
tests/unit_test/fuel/f3/streaming/download_service_test.py |
Replaces _accept_outcomes lock-discipline test with ordering/guard tests; adds serving-path callback exception-safety test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR delivers the complete F3/Cell payload layer: receiver-confirmed completion (served EOF is provisional until the receiver reports its finalization truth), per-receiver acquire/idle budgets with declared receiver identities, and the
Confidence Score: 5/5Safe to merge: the core concurrency invariants (activity gate, nonce binding, ownership guard, shutdown drain) are all well-designed and backed by targeted regression tests. The change is a large, carefully-designed protocol addition with extensive test coverage (60+ new tests) across the key concurrent edge cases. The one finding — that The settlement Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant S as Sender (DownloadService)
participant R as Receiver (confirm-capable)
participant W as TransferWaiter
Note over S: new_transaction() registers tx<br/>outcome_owners[tid] = tx<br/>get_transfer_waiter() → W parked in tx_waiters
S->>R: small control message (ref ids)
loop receiver-driven chunk pulls
R->>S: "request(rid, state, CONFIRM_CAPABLE=True)"
S-->>R: reply(chunk, next_state)
Note over S: mark_receiver_active updates<br/>_acquired_receivers + _receiver_last_active
end
R->>S: "request(rid, final_state, CONFIRM_CAPABLE=True)"
Note over S: obj_served(expect_confirm=True)<br/>→ _pending_confirms[r]=(SUCCESS,nonce)<br/>NOT yet in receiver_statuses
S-->>R: "reply(EOF, CONFIRM_EXPECTED=True, nonce)"
Note over R: receiver stores model<br/>(download_completed / file write)
R->>S: CONFIRM(rid, SUCCESS, nonce) [fire-and-forget]
Note over S: _handle_confirm → begin_op()<br/>obj_confirmed → _finalize_receiver<br/>(nonce matches → records SUCCESS)<br/>end_op()
Note over S: Monitor: is_finished()=True<br/>_delete_tx → _terminating_txs[tid]<br/>transaction_done(FINISHED)
Note over S: _drain_ops(60s): waits active_ops=0<br/>snapshot → compute_transfer_outcome → COMPLETED
Note over S: callbacks → source release<br/>THEN _record_outcome → outcome_owners.pop<br/>tx_waiters.pop → W._resolve(outcome)
W-->>S: waiter.wait() returns TransferOutcome(COMPLETED)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant S as Sender (DownloadService)
participant R as Receiver (confirm-capable)
participant W as TransferWaiter
Note over S: new_transaction() registers tx<br/>outcome_owners[tid] = tx<br/>get_transfer_waiter() → W parked in tx_waiters
S->>R: small control message (ref ids)
loop receiver-driven chunk pulls
R->>S: "request(rid, state, CONFIRM_CAPABLE=True)"
S-->>R: reply(chunk, next_state)
Note over S: mark_receiver_active updates<br/>_acquired_receivers + _receiver_last_active
end
R->>S: "request(rid, final_state, CONFIRM_CAPABLE=True)"
Note over S: obj_served(expect_confirm=True)<br/>→ _pending_confirms[r]=(SUCCESS,nonce)<br/>NOT yet in receiver_statuses
S-->>R: "reply(EOF, CONFIRM_EXPECTED=True, nonce)"
Note over R: receiver stores model<br/>(download_completed / file write)
R->>S: CONFIRM(rid, SUCCESS, nonce) [fire-and-forget]
Note over S: _handle_confirm → begin_op()<br/>obj_confirmed → _finalize_receiver<br/>(nonce matches → records SUCCESS)<br/>end_op()
Note over S: Monitor: is_finished()=True<br/>_delete_tx → _terminating_txs[tid]<br/>transaction_done(FINISHED)
Note over S: _drain_ops(60s): waits active_ops=0<br/>snapshot → compute_transfer_outcome → COMPLETED
Note over S: callbacks → source release<br/>THEN _record_outcome → outcome_owners.pop<br/>tx_waiters.pop → W._resolve(outcome)
W-->>S: waiter.wait() returns TransferOutcome(COMPLETED)
Reviews (17): Last reviewed commit: "Rename _leaked_ops to _terminating_txs: ..." | Re-trigger Greptile |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4865 +/- ##
==========================================
+ Coverage 60.28% 60.88% +0.59%
==========================================
Files 971 977 +6
Lines 92463 93503 +1040
==========================================
+ Hits 55745 56929 +1184
+ Misses 36718 36574 -144
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…, awaitable facade
The consolidated F3/Cell layer the Client API execution modes build on
(design: docs/design/client_api_execution_modes.md; plan F3-2 + F3-3 + F3-4).
Upper layers (executor backends, trainer engine) consume one primitive:
waiter = downloader.get_waiter()
outcome = waiter.wait(timeout=...) # returns == delivered
F3-2 Receiver-confirmed completion + retry-aware accounting.
A confirm-capable receiver's served EOF/ERROR is PROVISIONAL; the receiver's
fire-and-forget confirmation -- sent only after Consumer.download_completed()
finalization actually succeeds -- finalizes its status. Receiver truth wins
(served-EOF + failed finalization = confirmed FAILED, the disk-offload case);
retries overwrite provisional records; the first confirmation is final; a
confirmation is accepted only against a pending provisional serve on the same
ref incarnation, so stale/unsolicited confirms can neither certify nor poison.
All wire keys are optional -- both version skews degrade to today's
producer-served semantics -- and a runtime kill-switch
(streaming_receiver_confirm_enabled, standard application-config resolution)
disables the wire behavior on either side without a code revert.
F3-3 Per-(transfer, receiver) acquire/idle budgets + quorum surface.
Unconditional per-receiver activity tracking (a live receiver no longer masks
a stalled one behind the tx-wide timestamp). A receiver that never pulls
(acquire, vs workflow-declared receiver_ids, acquired at TRANSACTION level so
sequential multi-ref downloads are safe) or goes silent (idle) is finalized
FAILED on a monitor pass with a truth-wins re-check -- the aggregate outcome
resolves in bounded time, not the transaction TTL. This also bounds a lost
confirmation (fail-closed). Expected receiver identities now thread from
via_downloader into the transaction. min_receivers surfaces the k-of-N quorum
(quorum_met requires the same receiver to succeed on EVERY ref); `completed`
stays the strict all-receivers certificate.
F3-4 TransferWaiter, the awaitable facade.
Event-driven (resolved inside outcome recording; attaches before or after
termination), never hangs (unknown/expired/shut-down ids resolve immediately;
shutdown releases all waiters), optional FINISHED-gated linger preserving the
tombstone replay window for lost terminal replies, acquired_receivers() as the
V1 PAYLOAD_ACQUIRED signal, and composes with -- never replaces -- the
existing DOWNLOAD_COMPLETE_CB chain.
54 new unit tests (receiver_confirm_test, receiver_budget_test,
transfer_waiter_test): skew matrix, kill-switch both sides, receiver-truth-
wins, retry healing, mixed fleets, tombstone interplay, unsolicited-confirm
guard, bounded lost-confirm, multi-ref acquisition, quorum intersection,
config resolution, waiter never-hangs invariants. Full streaming+fobs+legacy
progress sweeps green (547). Reviewed by a 28-agent adversarial workflow; all
24 confirmed findings fixed.
…ed test helpers Cleanup pass over the F3 payload-layer commit (4 parallel review angles: reuse / simplification / efficiency / altitude). No behavior change. - CONFIRM_EXPECTED is serialized only on terminal replies: since confirmations are sent only after terminal serves, the per-chunk advertisement was dead weight on the hottest wire message. Terminal branch folded to one if/else. - Acquisition promoted to the transaction (_acquired_receivers, monotonic set with a double-checked add: at most one lock per (transaction, receiver), not per chunk). Replaces the per-pass union rebuild across refs in both enforce_receiver_budgets and get_acquired_receivers -- acquisition is a transaction-level fact, now modeled as one. - Monitor budget pre-pass pre-filters to transactions that actually have budgets (has_receiver_budgets), so the common no-budget case does zero per-transaction lock reacquisitions per tick. - _resolve_receiver_budget now reuses get_positive_float_var and check_positive_number instead of hand-rolling both branches. - Kill-switch comment corrected: per-process (read once; restart to change), not "runtime". Redundant pending-confirm pop in enforce_budgets removed (_finalize_receiver owns it). - Test scaffolding deduplicated: the pull/confirm wire builders, pull_to_terminal loop, and monitor-suppressed service factory move to download_test_utils; the confirm kill-switch fixture moves to a shared conftest.py. Removes five helpers copy-pasted across three files. Deliberate skips: lru_cache for the kill-switch cache (the module global is a load-bearing test patch point), StreamFuture delegation for TransferWaiter (different resolve semantics; minimal savings), spec-object for the transaction kwargs and a general capability-negotiation mechanism (premature until more consumers/capabilities exist -- noted for later).
The plan file was a point-in-time PR decomposition, not a durable reference: execution has already deviated from it deliberately (the F3 track shipped as one consolidated PR; the trainer-engine track was re-scoped), and keeping it current would mean plan-only doc churn with every such decision. A stale plan misleads more than no plan; git history preserves it for archaeology. The design doc -- the durable contract reference -- stays, with its plan references decoded into work-item names.
a93eb6a to
a98d74a
Compare
Behavior-free rename for clarity. The map's purpose is ownership: the transaction object currently entitled to record the outcome for its tx_id. tx_ids are deliberately reused by retries (design: stable transfer ids make retries idempotent), so recording checks object identity against this map -- "if owner is not tx: drop" now reads as what it means. "Incarnation" named the mechanism; "owner" names the purpose. Comments, docstrings, and test names updated; ref-level wording uses "life of the ref" (refs have no owner table; the pending-confirm guard plays that role).
F3-2/F3-3/F3-4 and "plan:" references decoded against an implementation-plan document that no longer exists; the feature names say what the ids only pointed at. Comments now name the mechanisms directly (receiver-confirmed completion, per-receiver budgets, awaitable transfer facade).
…ore resolving waiters Fixes a reproduced review batch on the F3 payload layer. Declared receiver identities are now enforced end to end. Completion (is_finished, the downloaded_to_all latch), the aggregate outcome, and the quorum are judged against receiver_ids when declared: a status from an unexpected receiver can no longer complete -- or certify -- a transfer that a declared receiver never got. Count-based semantics remain only when identities are unknown. (Also makes a ref-less transaction never "finished".) Confirmations are bound to their serve by a per-serve nonce. The terminal reply carries CONFIRM_NONCE; the confirmation must echo it. The pending-entry check alone could not distinguish ref lives: a delayed confirmation from a previous life of a reused ref_id was accepted whenever the new life had its own pending serve for the same receiver. Confirmations also now honor the download's secure flag. Ownership and table registration are one atomic step (_tx_lock nesting _outcome_lock; no reverse nesting exists). Two concurrent same-id constructors could previously interleave across the separate critical sections, leaving the live transaction without outcome ownership while the retired one recorded its DELETED outcome as the live attempt's verdict. Outcomes are recorded -- and waiters released -- only after the transaction settles: terminal progress, done/outcome callbacks, and source release all complete before wait() can return, so an upper layer that stops the producer on wait() return can never preempt them. Idle budgets judge transaction-level activity: a receiver that finished one ref and went silent was exempt from a sibling ref's acquire budget (tx-acquired) while having no per-ref idle timestamp there -- escaping both budgets and pinning the producer to the full TTL. Superseded transactions (tx_id reuse retirement) release their sources but suppress transfer-facing emissions (terminal progress, transaction_done_cb, outcome_cb): their reused tx_id names the live retry, and consumers keyed by tx_id would misattribute them. Regression pins for every reproduced scenario: unexpected-receiver certification, cross-life stale confirmation (with and without a new pending serve), concurrent same-id creation hammer, settle-before-wait-return, multi-ref budget escape, superseded-callback silence, secure confirmations.
…ore resolving waiters Fixes a reproduced review batch on the F3 payload layer. Declared receiver identities are now enforced end to end. Completion (is_finished, the downloaded_to_all latch), the aggregate outcome, and the quorum are judged against receiver_ids when declared: a status from an unexpected receiver can no longer complete -- or certify -- a transfer that a declared receiver never got. Count-based semantics remain only when identities are unknown. (Also makes a ref-less transaction never "finished".) Confirmations are bound to their serve by a per-serve nonce. The terminal reply carries CONFIRM_NONCE; the confirmation must echo it. The pending-entry check alone could not distinguish ref lives: a delayed confirmation from a previous life of a reused ref_id was accepted whenever the new life had its own pending serve for the same receiver. Confirmations also now honor the download's secure flag. Ownership and table registration are one atomic step (_tx_lock nesting _outcome_lock; no reverse nesting exists). Two concurrent same-id constructors could previously interleave across the separate critical sections, leaving the live transaction without outcome ownership while the retired one recorded its DELETED outcome as the live attempt's verdict. Outcomes are recorded -- and waiters released -- only after the transaction settles: terminal progress, done/outcome callbacks, and source release all complete before wait() can return, so an upper layer that stops the producer on wait() return can never preempt them. Idle budgets judge transaction-level activity: a receiver that finished one ref and went silent was exempt from a sibling ref's acquire budget (tx-acquired) while having no per-ref idle timestamp there -- escaping both budgets and pinning the producer to the full TTL. Superseded transactions (tx_id reuse retirement) release their sources but suppress transfer-facing emissions (terminal progress, transaction_done_cb, outcome_cb): their reused tx_id names the live retry, and consumers keyed by tx_id would misattribute them. Regression pins for every reproduced scenario: unexpected-receiver certification, cross-life stale confirmation (with and without a new pending serve), concurrent same-id creation hammer, settle-before-wait-return, multi-ref budget escape, superseded-callback silence, secure confirmations. Additionally, a property-falsification pass over this batch (six design guarantees, each attacked with executed counterexample code; 74 attacks run) found one more instance of the same class: shutdown() cleared _tx_table and _outcome_owners in separate critical sections, so a new_transaction landing in the gap had its ownership wiped while staying live in the table -- outcome unrecordable forever, waiter falsely resolved None. Teardown is now one atomic step (_tx_lock nesting _outcome_lock, matching new_transaction). The regression pin discriminates on lock state, not timing, and fails in milliseconds against the pre-fix code. The other five properties (identity certification, confirm-serve binding, settle-before-resolve, bounded silence, fail-closed) held under attack.
Three follow-up review findings on the settlement path: A raising custom Downloadable.release() could escape transaction_done() -- and with outcome recording moved to the end of settlement, that left the outcome unrecorded, the waiter pending forever, ownership registered, and killed the monitor thread. Releases are now individually guarded and recording runs in a finally block: no exception anywhere in settlement can prevent the verdict from being recorded and waiters released. A retry could take ownership of a tx_id while the previous transaction was mid-settlement (already popped from _tx_table by the monitor, callbacks not yet run): the retire path never saw it, so its stale transfer-facing emissions ran against the retry's id. new_transaction now marks a previous owner superseded when taking ownership, and transaction_done re-reads the superseded state at each emission gate. Superseded suppression was also over-broad: transaction_done_cb is a CLEANUP surface in practice (in-tree callers delete temp files there, e.g. workspace_cell_transfer._cleanup_transfer_files) and skipping it leaked files on active tx_id reuse. Cleanup surfaces (transaction_done_cb, obj.transaction_done, release) now always run; only the transfer-facing emissions (terminal progress, outcome_cb) are suppressed for superseded transactions. Pins: raising release resolves the waiter and consumes ownership off the monitor thread; mid-settlement retry suppresses outcome_cb/progress while cleanup still runs; retirement fires the cleanup callback but not outcome_cb.
Follow-up review found the supersede mechanism unsound in both directions: the boolean gate is a check-then-emit race (a retry can take ownership after the old generation passes the check but before the callback fires -- there is no safe interleaving without holding a lock across user callbacks), and always-running transaction_done_cb misattributes: it is a completion surface (FOBS DOWNLOAD_COMPLETE_CB), not just cleanup, so an old generation reporting DELETED under an id a live retry owns is wrong. Both dissolve under strict generation serialization: new_transaction may not take ownership of a reused tx_id until every previous generation has fully settled. A live prior transaction is retired and settled inline; a mid-settlement one (popped from _tx_table, callbacks in flight) is awaited on its settled event, set in transaction_done's finally after outcome recording. Every terminal emission of an old generation therefore happens strictly before its retry exists -- nothing needs to be suppressed, and the superseded flag, its gates, and the retire special case are deleted. Ownership entries in _outcome_owners now double as the mid-settlement markers; recording consumes them. Guards: a settlement callback that synchronously reuses its own tx_id gets an immediate error instead of a self-deadlock; a retry waiting on a hung settlement fails loudly after a bounded wait (60s). Property falsification against the new design found and fixed two gaps before review: a settled-but-unconsumed owner (outcome recording itself failed) was reclaimed-never -- Event.wait on a set event returns True instantly, so the bounded-wait escape was dead code and a retry hot-spun on the global locks forever; owners are now reclaimed when already settled. And shutdown() cleared ownership markers before its deferred settlements ran, so a same-id retry in that window registered while old-generation emissions were still to come; shutdown now keeps the markers (settlement consumes them) and drops verdicts via a per-transaction record-forbidden flag instead -- nothing records after shutdown, waiters still resolve. Also: deleted dead _Transaction.timed_out() (settled without unlinking from _tx_table -- the one path that could double-settle); outcome computation moved inside try so a failure there cannot poison the tx_id; the settlement wait deadline re-arms per generation; waiter semantics documented and pinned (waiters bind to the generation that records while they wait). Scope note, documented in code and description: serialization covers the terminal surfaces. A data-plane event already executing at retirement (an in-flight serve's produce(), a monitor budget pass) may finish after the retry registers; stragglers cannot alter any recorded verdict.
…aits Three follow-up review findings on generation serialization: The settlement bound did not cover retiring a LIVE prior transaction: that path settled inline on the caller's thread, so a hung callback in the old generation hung the retry forever -- the 60s bound only applied to generations already settling elsewhere. Retirement now settles on its own (abandoned-on-timeout, daemon) thread and the retry takes the same bounded wait as any mid-settlement predecessor; its ownership marker is consumed whenever the settlement eventually finishes, so a later retry can succeed. Serialization only covered emissions made inside transaction_done: an in-flight monitor budget pass -- table-identity check, lock released, then enforcement -- could still emit FAILED terminal progress under the reused tx_id after a retry retired, settled, and registered. The same window existed for in-flight serves and confirms. Operations now register with a per-transaction activity gate (begin_op/end_op); settlement closes the gate and drains registered operations BEFORE snapshotting the outcome -- late finishes are counted in the verdict, and nothing already executing can emit after settlement (and therefore after a same-id retry) anymore. An operation that finds the gate closed treats the transaction as gone (same as a missing ref). Only a user callback hanging beyond the 60s drain timeout can leak a late emission; settlement then proceeds with a loud warning. Reclaiming a dead owner (settled but its recording step failed) no longer lets that generation's parked waiters be inherited -- and later resolved -- by the retry's outcome: they are abandoned with None at reclaim, exactly like shutdown does. Waiters stay generation-bound. Pins: hung retirement fails the retry within the bound and the id recovers once the abandoned settlement finishes; an in-flight budget pass blocks the retry until drained, with every old-generation emission (including enforcement's FAILED progress) strictly before registration; reclaimed owners abandon stranded waiters with None. Re-ran the falsification attack suite: budget-path and serve-path counterexamples now hold, 0/200 hammer violations (was 42/200), deadlock and single-shot properties still hold, shutdown races still hold.
shutdown() resolves the waiters it sees and keeps ownership markers so same-id retries serialize behind the deferred settlements -- but that surviving marker makes the tx_id look live to get_transfer_waiter: a waiter created in the window between shutdown's locked teardown and a deferred settlement consuming ownership was parked, and the record-forbidden branch of _record_outcome then consumed ownership without draining it. The waiter hung forever, violating "waiters never hang". The forbidden branch now abandons any window waiters with None, the same verdict shutdown gave the waiters it saw. Every path that consumes ownership now drains waiters: record resolves with the outcome, reclaim and the forbidden branch abandon with None, shutdown resolves wholesale. Pin holds the deferred settlement open with a gated callback, parks a waiter mid-window, and asserts it resolves to None; verified to fail (bounded) with the drain removed.
…reuse Five review rounds attacked the reused-tx_id retry design; four falsified its generation boundary (mid-settlement supersede, flag-gated suppression races, unbounded retirement, in-flight budget-pass emissions, shutdown window waiters). Each fix was correct, but the pattern was defending a boundary that naming can delete: when two attempts never share an id, a late emission from a dying attempt names that attempt truthfully forever, and misattribution becomes unrepresentable instead of prevented. Grounding facts: the tx_id never crosses the wire in the pull protocol (receivers pull by ref_id), so retry idempotency always belonged to the application-level transfer id carried in caller metadata -- in any design. And id reuse never existed as behavior: the tx_id parameter has been dormant since introduction (every caller takes a fresh uuid). The reuse contract was a plan for the not-yet-written trainer engine, with zero consumers to migrate. new_transaction now registers in one atomic step and raises ValueError on a duplicate id while it is live, settling, or its receipt is retained (TX_OUTCOME_TTL bounds the exclusion window for caller-supplied ids). Deleted with the reuse contract: generation serialization (settled events, mid-settlement waits, settler threads, SETTLEMENT_WAIT_TIMEOUT, the settlement-callback reentrancy guard), dead-owner reclaim and its waiter abandonment, receipt-wipe-on-registration, and the record_forbidden marker dance in shutdown (which reverts to the simple atomic clear; the owner guard alone gates post-shutdown recording, and a waiter arriving in the deferred-settlement window now resolves None immediately instead of parking). Waiters trivially bind to the single attempt their id names. Kept, rationale unchanged: receipts and TTL, TransferWaiter, per-receiver budgets, receiver confirms including the per-serve nonce (ref_id reuse via PASS_THROUGH re-emission is independent of retries), tombstones, the settle-then-resolve ordering, exception-guarded callbacks, and the activity gate -- demoted from misattribution defense to verdict completeness: an operation finishing during termination is counted in the outcome, and nothing emits against a settled transaction. The duplicate check expires receipts inline, so the exclusion window for a caller-supplied id is exactly TX_OUTCOME_TTL (not TTL plus a sweep cycle). The execution-modes design doc is aligned: tx_id never equals transfer_id; each attempt gets a fresh download_tx_id and the manifest carries the transfer_id mapping. Tests updated to pin the new contract: duplicate ids rejected across the whole lifetime (live, settling, receipted) and freed by receipt expiry; racing same-id constructors resolve to exactly one winner and clean errors; settlement callbacks may create new transactions but not their own id; a recording failure leaves the id excluded and shutdown releases its waiter; shutdown-window waiters resolve immediately; the in-flight budget-pass drain now pins that enforcement verdicts are counted in the receipt. Net -293 lines.
Review found the composition gap in the attempt-scoped duplicate check: a settlement drain that times out (OP_DRAIN_TIMEOUT) deliberately proceeds with an operation still in flight, and a caller-supplied tx_id becomes registrable again once its receipt expires (TX_OUTCOME_TTL) -- so a leaked operation that resumes after both windows could emit progress/failure under an id that now names a NEW transaction. Every terminator now registers a drain-timeout leak (tx with in-flight operations after settlement) in _leaked_ops; new_transaction rejects the id -- independently of receipt state -- until the leaked operation count reaches zero, and the monitor reaps entries whose operations exited. The id becomes reusable only after receipt expiry AND all old operations exited; the default fresh uuid never collides with anything. Description wording tightened to match the implementation: ids are single-use while known (live, settling, receipted, or leaked), not "never reused, forever". Pin: an operation held past a shortened drain timeout keeps the id rejected even with the receipt force-expired, and the id frees the moment the operation exits.
Comment/docstring share of download_service.py had grown to a third of the file across the review rounds -- much of it review-history narration and correctness arguments addressed to reviewers, which are noise once merged (that knowledge lives in commit messages and the regression pins). Rules applied: comments state constraints the code cannot show (lock ordering, unlink-before-settle, why callbacks run outside locks, wire semantics); each invariant is documented at exactly one home (table invariants on the class attributes, contracts in method docstrings) with other sites shortened to a pointer; review archaeology and justification-of-change prose deleted. Public API docstrings and test docstrings (each pin documents the bug it guards) are untouched.
Review found the leak exclusion's shutdown gap: shutdown() clears the ownership and receipt exclusions up front, but leak registration ran only AFTER the deferred settlement -- so a drain-leaked operation's tx_id looked free for that whole window (unbounded if a settlement callback hangs), and a replacement could register while the leaked operation was still able to emit under the id. delete_transaction and the monitor were not affected: their ownership marker covers settlement and their receipt covers the tail. shutdown() now pre-registers every registered owner (live and mid-settlement) as a potential leak in the same atomic teardown section, before clearing ownership. The per-terminator hook becomes a sync (_update_leaked_ops): it keeps the exclusion while operations are in flight and releases it once none are, so pre-registered ids free normally after a clean drain. Pin drives the exact reviewer sequence: leaked op + shutdown mid-settlement -> same-id registration rejected, still rejected after shutdown returns, freed the moment the operation exits. Also from review: explicit pin that a CONFIRM-keyed message dispatches to confirm handling and never falls through to pull handling (a fall-through would re-serve from scratch); duplicate receiver_ids now log a warning (almost certainly a caller bug) with the dedup pin extended to assert it; make_confirm_test_service renamed to make_service_no_monitor (it was never confirm-specific); conftest notes that its autouse confirm-switch fixture is directory-wide.
Review round 8 falsified the shutdown pre-registration's release condition: a marker was releasable whenever _active_ops == 0, but settlement callbacks are not operations -- so with the old transaction blocked inside transaction_done_cb (zero ops), a same-id constructor could pop the marker mid-settlement, register, and then the old outcome_cb fired under an id naming the replacement. The marker is now two-state, as the reviewer prescribed: releasable only when the transaction's settlement has completed (a flag set in transaction_done's finally, after outcome recording -- nothing emits after it) AND no operations are in flight. All three release paths enforce the conjunction: the duplicate check in new_transaction, the per-terminator _update_leaked_ops sync, and the monitor reap. A registration that slips between the flag and the terminator's sync is legal (settlement is fully done), and the sync's identity check keeps it from ever touching the replacement's state. Pin drives the reviewer's exact clean-settlement sequence: shutdown with a gated transaction_done_cb and zero operations -> same-id registration rejected mid-settlement, accepted once settlement completes.
Review round 9 composed two timing facts into an exposure: the outcome timestamp is captured before the settlement callbacks run, so a settlement slower than TX_OUTCOME_TTL recorded a receipt that was expired at birth -- and the inline expiry check then handed the id to a same-id constructor in the gap after ownership was consumed but before the terminator's marker sync ran. The old transaction's marker then landed on top of the live replacement while its leaked operation could still emit. Two changes, per the reviewer's recommendation: The termination marker is now installed by _delete_tx itself, atomically with the unlink, for EVERY terminator -- not just shutdown. One mechanism covers the entire termination window (settlement plus any drain-leaked tail) on every path; the ownership and receipt exclusions still exist but no longer carry any window alone. Shutdown's separate owner pre-registration loop became redundant and is removed. The overlap state can no longer form: the marker exists before ownership is consumed, and releases only on settlement-complete AND zero operations. Receipts are re-stamped at recording time: the TTL retention window starts when the receipt becomes queryable, matching the documented "kept 30 min" contract, so a slow settlement can never record an already-expired receipt. Pins: slow-settlement receipt is fresh from recording and excludes the id; the reviewer's aged-receipt-plus-leaked-op sequence stays excluded end to end with no overlap; the marker exists mid-settlement on the delete path (not just shutdown).
Greptile flagged that the settlement finally's stated invariant -- no exception may leave the outcome unrecorded and waiters pending -- was not upheld if compute_transfer_outcome itself raised: outcome stayed None, recording was skipped, ownership was never consumed, and a waiter parked on the id hung until shutdown (the dead-owner reclaim that once bounded this left with the attempt-scoped rework). Unrealistic in practice (dataclass construction over simple types), but the invariant is now enforced rather than assumed: the finally builds a fail-closed fallback verdict (empty refs certify nothing) whenever outcome is None, recording always runs, and settlement never propagates an exception to its caller (a raise on the monitor thread would have killed the monitor and skipped the terminator's marker sync). Also from the same review: _reap_leaked_ops now reads the op count and settlement flag under the per-transaction gate lock (_tx_lock -> _ops_cond, the established order) instead of relying on CPython read atomicity. Pin: compute_transfer_outcome patched to raise -> waiter still resolves with a fail-closed outcome, ownership consumed, no exception escapes the terminator.
…erdict Review round 10 falsified the previous fallback twice over: a one-shot outcome-computation failure skipped every cleanup emission and the source release (waiter resolved, 70GB pinned), and a persistent failure escaped the finally because the fallback REUSED the failing function -- stranding the waiter, leaving ownership and the termination marker unreleasable, and able to kill the monitor thread. The regression pin masked both by raising only on the first call and never asserting release. The ceremony is now three independent phases: Verdict: computed under its own guard; a failure is contained and no longer skips anything downstream. Emissions that need the verdict (outcome_cb) are gated on having one; cleanup emissions (obj.transaction_done, transaction_done_cb, terminal progress) run regardless. Source release: its own finally phase -- no failure in the verdict or the callbacks can pin the sources in memory. Recording: runs last and cannot raise. The fallback is direct fail-closed dataclass construction (never compute_transfer_outcome), each step is guarded, and the settlement-complete flag is set unconditionally, so ownership is consumed, waiters resolve, and the termination marker stays releasable no matter what happened above. Codex review refinements on the same patch: the fail-closed verdict is built in the computation exception handler itself (a _fail_closed_outcome helper) with full transaction metadata and a dedicated COMPUTATION_FAILED reason, and flows through outcome_cb like any verdict -- the callback contract holds on this path; the recording-phase fallback remains as a last-resort belt via the same helper. Codex round 2 returned clean and contributed test-only additions, kept: the fail-closed outcome preserves min_receivers/receiver_ids for both outcome_cb and the waiter, and outcome_cb runs before source release. Pin hardened to the reviewer's spec: compute_transfer_outcome raising PERSISTENTLY -> waiter resolves fail-closed, ownership consumed, sources released, per-object cleanup fired, no exception escapes the terminator, and a new transaction registers cleanly afterward.
The dict began (review round 7) as a registry of drain-LEAKED OPERATIONS; rounds 8-9 grew it into the general termination marker installed at unlink for every terminator -- most entries never involve a leak, and the values are transactions, not operations. The name now says what the duplicate check reads it as, matching its error message: the id's previous transaction "has not fully terminated". _update_leaked_ops -> _sync_termination_marker (release or sustain after settlement), _reap_leaked_ops -> _reap_termination_markers. Mechanical rename; no behavior change. The isolated-service test factory override is renamed with it, keeping test isolation intact.
The complete F3/Cell payload layer the Client API execution modes build on, plus the design revision that documents it. Upper layers (executor backends, trainer engine) consume one primitive:
(This PR ships the primitive and the truth model behind it; production callers of
get_waiter()arrive with the trainer-engine and executor-backend PRs. Nothing user-visible changes yet.)How a huge-model send flows: before and after
Before this PR — served is assumed delivered. The sender's books say SUCCESS when the last chunk leaves; whether the receiver could actually store the model is unknowable, and the terminal FINISHED status counts failed receivers too:
sequenceDiagram participant S as sender cell (holds 70GB model) participant R as receiver cell Note over S: flare.send() registers refs, then waits on ad-hoc<br/>machinery: progress messages, timeout guesses, ack grace. S->>R: small control message (ref ids, no payload) loop receiver-driven chunk pulls R->>S: request(R1, state) S-->>R: reply(chunk, next state) end R->>S: request(R1, final state) S-->>R: reply(EOF) Note over S: sender counts this receiver as SUCCESS<br/>the moment the last chunk is served. Note over R: receiver stores the model (e.g. disk write).<br/>If this fails, nobody tells the sender. Note over S: transaction FINISHED means receiver COUNT reached,<br/>counting failed receivers too.<br/>No queryable verdict exists afterward. Note over S,R: consequences: the sender frees the model or exits its<br/>subprocess on a guess. A receiver whose store failed is<br/>invisible: the workflow proceeds without the model (silent truncation).With this PR — served ≠ delivered; the receiver's own stored/failed report decides. Every receiver's outcome is knowable, bounded in time, and queryable afterward:
sequenceDiagram participant S as sender cell (holds 70GB model) participant R as receiver cell Note over S: sender registers transaction T0 with refs R1..Rn.<br/>The model stays in sender memory - only refs travel.<br/>waiter = get_transfer_waiter(T0) is the facade this PR adds. S->>R: small control message (ref ids, no payload) loop receiver-driven chunk pulls (MBs per chunk) R->>S: request(R1, state, CONFIRM_CAPABLE) S-->>R: reply(chunk, next state) end R->>S: request(R1, final state, CONFIRM_CAPABLE) S-->>R: reply(EOF, CONFIRM_EXPECTED) Note over S: sender notes: all bytes served to this receiver,<br/>outcome pending the receiver's confirmation.<br/>Not yet counted as delivered. Note over R: receiver stores the assembled model<br/>(download_completed, e.g. write the file to disk).<br/>This step can fail even though every byte arrived. R->>S: CONFIRM: stored OK / store failed (fire and forget) Note over S: sender records what the receiver reported.<br/>stored OK: counted as delivered.<br/>store failed: counted as failed, despite the served EOF. Note over S: when every expected receiver is counted, the monitor closes T0:<br/>callbacks run, 70GB source release attempted (errors logged), THEN the verdict is recorded<br/>(kept 30 min) and waiter.wait() returns - fully settled. Note over S,R: bounds on the sender's 5s monitor tick. Budgets are opt-in,<br/>set per transaction or via config vars - acquire needs declared receiver ids.<br/>receiver never pulls: failed at the acquire budget.<br/>receiver goes silent or its CONFIRM is lost: failed at the idle budget.<br/>either budget unset: the transaction TTL remains the backstop.<br/>old-version receiver (no CONFIRM_CAPABLE): counted at EOF, exactly today's behavior.The trade-off, stated plainly
Holding the source: the sender keeps the model longer only on the happy path (+ receiver store time + one confirm hop) — exactly the window in which the old code had already freed a model whose delivery was unproven. In the degraded cases the hold is shorter than before:
The budget rows apply when budgets are configured (opt-in, per transaction or via
streaming_receiver_acquire_timeout/streaming_receiver_idle_timeout; the acquire budget additionally requires declared receiver identities). Unconfigured deployments keep exactly the before-column TTL behavior.Network hiccups: chunk/EOF loss is handled as before (retries, tombstone replay). The one new must-arrive message is the CONFIRM; if it is lost, the receiver is counted FAILED despite a good store — a false negative, bounded by the idle budget, healed by an idempotent retry. This is the Two Generals bound: some message is always last and unconfirmed, so a protocol only chooses which way it errs. Before, we erred toward invisible false success (silent truncation); now we err toward visible, bounded, retryable false failure. Corruption-by-assumption is no longer a possible outcome.
Outcome-recording hardening (follow-up to #4853's F3-1): tx_ids are attempt-scoped and single-use while known — a retry of the same logical transfer is a new transaction with a new id, and
new_transactionrejects a duplicate id (ValueError) while it is live, still settling, its receipt is retained (TX_OUTCOME_TTL), or a drain-timeout leak from a previous attempt is still in flight. The default fresh uuid never collides; a caller-supplied id becomes reusable only after its receipt expired AND every old operation exited. The stable cross-attempt identity is the application-level transfer id carried in the caller's metadata; it never enters this service. Uniqueness is what makes every emission of a terminated transaction attributable to it alone — no suppression flags, generation serialization, or settlement waiting exist (an earlier revision serialized reused-id generations; review falsified that boundary repeatedly, so the boundary was deleted at the naming level). Ownership (_outcome_owners) and_tx_tableregistration remain one atomic step, so a transaction the monitor can terminate always owns its outcome slot, concurrent same-id constructors resolve to exactly one winner and clean errors for the losers, andshutdown()clears ownership atomically with table teardown (nothing records after shutdown; the owner guard alone gates it). In-flight operations (serves, confirms, monitor budget passes) register with a per-transaction activity gate that settlement closes and drains before snapshotting the outcome — an operation finishing during termination is counted in the verdict, and nothing emits against a settled transaction (only a callback hanging beyond the 60s drain timeout can leak a late emission, loudly logged — and such a leak keeps its tx_id excluded from registration until the operation exits, so a leaked emission can never land under a recycled id);txrequired in_record_outcome(recording is legal only for the slot owner); deep-frozenTransferOutcome(note:receiver_statusesis aMappingProxyType— not JSON/pickle-serializable by design; consumers crossing a boundary materialize withdict(...)); exception-guardeddownloaded_to_*callbacks.Receiver-confirmed completion + retry-aware accounting: a confirm-capable receiver's served EOF/ERROR is provisional; the receiver's fire-and-forget confirmation — carrying its stored-OK / store-failed truth (a finalization failure confirms FAILED) — finalizes it. Receiver truth wins (served-EOF + failed store = FAILED — the disk-offload case); retries overwrite provisional records. Every confirmation echoes a per-serve nonce from the terminal reply, binding it to its exact serve: a stale confirmation from a previous life of a reused ref_id is dropped even when the new life has its own pending serve for the same receiver. Confirmations ride with the download's
securesetting. All wire keys optional (both version skews degrade to today's producer-served semantics); per-process kill-switchstreaming_receiver_confirm_enableddisables the wire behavior on either side without a code revert.Declared receiver identities + per-(transfer, receiver) budgets + quorum: when
receiver_idsare declared, completion, the aggregate outcome, and the quorum are judged against those identities — a status from an unexpected receiver can never complete a transfer a declared receiver did not get. Budgets (opt-in): a never-pulling receiver fails at the acquire budget (requires declared identities); idleness is judged on transaction-level activity, so a receiver that finished one ref and went silent cannot escape a sibling ref's budgets — with a truth-wins re-check on enforcement. Budget failures resolve the outcome on a monitor pass instead of the TTL, which also bounds lost confirmations (fail-closed).min_receivers/quorum_metgive fan-out workflows the k-of-N surface (a quorum receiver must be a declared receiver and succeed on every ref);completedstays the strict all-receivers certificate.TransferWaiterawaitable facade: event-driven and settle-then-resolve — the outcome is recorded (and waiters released) only after the callback chain and the source-release attempts complete (a raisingrelease()is logged and cannot block settlement or strand the waiter), so acting onwait()returning can never preempt them. Never hangs (unknown/expired/shut-down ids resolve immediately; shutdown releases all waiters). tx_ids are attempt-scoped, so a waiter always resolves with the verdict of exactly the attempt it named; a retrying caller acquires a new waiter for the new attempt. Optional caller-specified linger (FINISHED-gated) preserves the tombstone replay window;acquired_receivers()is the V1 PAYLOAD_ACQUIRED signal; composes with — never replaces — the existingDOWNLOAD_COMPLETE_CBchain.Design rev 2.2 + vocabulary (folded from the former #4866): forward-path heartbeat exemption narrowed to the payload-materialization phase via new
Topic.TASK_PAYLOAD_READY(heartbeats stay authoritative during training; diagrams re-validated);launch_once=Falsemoves to launch-scoped tokens; F3 dependency/vocabulary-mapping updates; status set to Approved. The implementation-plan doc is removed — it was a point-in-time PR decomposition that execution has already deviated from; the design doc is the durable reference.Tests: 60+ new unit tests across
receiver_confirm_test,receiver_budget_test,transfer_waiter_test, and the F3-1 hardening suite — skew matrix, kill-switch both sides, receiver-truth-wins, retry healing, mixed fleets, tombstone interplay, unsolicited-confirm guard, bounded lost-confirm, multi-ref acquisition, quorum intersection, waiter never-hangs invariants. Full streaming+fobs+legacy sweeps green. Reviewed by adversarial multi-agent workflows (24 + 6 confirmed findings fixed) plus a 4-angle cleanup pass (net −48 LOC, hot-path trims).