fix(live): halt sweep read envelopes + latch only after attempted side effects - #1244
Conversation
MCPServerAdapter.call_tool returns {"status": "error", ...} envelopes
instead of raising, and the sweep's open-orders/positions reads iterated
them as mappings — AttributeError: 'str' object has no attribute 'get',
killing the kill-switch action for the episode. HKUDS#1232 taught the submit
responses about envelopes; the reads never learned.
_read_broker_state now validates every read before iteration: an error
envelope, a raised exception, or a non-list result becomes a structured
report['errors'] item (phase read_open_orders / read_positions) and the
phase aborts with no side effects. Non-dict entries in an otherwise-valid
list are skipped with an error item. Successful adapter responses are
unwrapped with pinned per-read keys (data.orders / data.positions;
data-as-bare-list accepted) — wrapper dicts without the pinned key are
rejected so metadata can never become fake broker records.
Regression tests use the real adapter envelope shape for both read paths,
plus non-list/invalid-entry rejection.
…ds succeeding Two behaviours, one root: the no-retry latch must reflect what the sweep actually did — and which halt episode the kill switch is for. 1. Latch timing: mark_sweep_fired ran before flatten_and_cancel, so a sweep whose reads both failed still persisted the latch and silently skipped the kill action for the episode — neither a later tick nor a restart retried it. flatten_and_cancel now reports side_effects_attempted (set the moment any cancel/close submit is tried); the runner latches iff any broker write was attempted (even when another read phase failed), keeps retrying only when nothing was attempted, and still latches conservatively on a raising sweep. Reads are additionally normalized at the boundary in api/live_routes._read (error envelopes raise, ok payloads unwrap to records/balance), which protects the shared reconcile path from the same envelope class. 2. Episode precedence: _halt_episode ranked the broker vs global sentinel by filesystem mtime, which ties within one timestamp quantum and let a stale per-broker latch suppress the sweep for a newer global HALT (halt_flag_set treats the global as authoritative for every channel). The explicit tripped_at payload now wins the ranking (mtime only as fallback for hand-touched sentinels), and the latch record accumulates every fired episode so clearing one halt never re-fires an older, already-swept one. Tests: restart-replay on read failure (no side effects), restart- suppression after an attempted close even with a read error (the duplicate-close hazard), newer-global rearm with forced-identical mtimes, nothing-to-do re-check policy, boundary normalization + envelope-raise wiring.
ebf5e18 to
5558f38
Compare
…ker errors Safety hardening for PR HKUDS#1244 — three gaps found outside the two reproduced behaviours: 1. Concurrent runner race (major): the sweep was check-then-act — sweep_already_fired checked, broker writes issued, durable marker written later. Two runner processes could both pass the check, both submit closes, then both write the latch. The whole check -> sweep -> durable-record window now runs under an exclusive disk claim (FLATTEN_CLAIM, O_CREAT|O_EXCL: exactly one winner). A held claim (another runner active, or a crash mid-sweep) means the episode's outcome is unknowable: never re-sweep — audit for operator resolution, HALT stays tripped. The claim is released in finally on every path. 2. Durable-latch persistence failure (major): a mark_sweep_fired fs error after a broker write left only the in-memory flag, so a restart could replay the close. The failure is now logged and audited ("restart may replay; verify broker state") — plus the claim protocol bounds the window it can happen in. 3. Nested broker error envelope (high-risk edge): _error_envelope_message checked only the top level. A successful transport carrying a broker-level rejection ({"status": "ok", "data": {"status": "error" | "ok": false}}) was recorded as accepted — order stays live while the sweep latches. Both nested shapes are now detected for cancel and flatten. Tests: claim exclusivity + release, claim-held runner skip (the concurrent-race reproduction), claim release on success and on read failure, nested-error rejection for both phases (order NOT recorded as cancelled, close NOT recorded as submitted). 88 live tests pass; ruff clean.
…ture trips Self-review found a hole in the claim protocol: the inter-process claim was not bound to a halt episode. A process crashing after mark_sweep_fired but before release_claim left an orphan claim that blocked EVERY future episode of that broker — a new trip would see claim_sweep fail and skip the kill action forever (reproduced). Claims are now episode-keyed (FLATTEN_CLAIM-<sha256(episode)[:16]>): each halt episode gets its own claim namespace. A crash claim can only block its OWN episode (the correct unknowable-outcome case — re-sweeping could duplicate a close) and never a later trip. halt_episode() is exposed publicly so the runner binds claim/release to the same episode it resolves for the latch (fallback 'unknown' when no sentinel payload is readable). Tests: orphan-claim regression (episode 1 crashed while claimed + never released -> episode 2 still claims and sweeps), plus updated exclusivity/release/skip tests for the episode-keyed API; claim files of other processes are never released. 89 live tests pass; ruff clean.
…e-failure branch Resolves both non-blocking residuals from the independent review: A. mark_sweep_fired recorded only the NEWEST halt episode. With an older global + newer broker halt both active, clearing the broker halt left the still-active global episode unrecorded, so sweep_already_fired returned False and the sweep re-fired (redundant no-op re-sweep + audit noise) — contradicting the module's documented semantics. _active_episodes now enumerates every tripped sentinel identity (shared _sentinel_identity helper), and mark_sweep_fired appends ALL of them to the latch record; the legacy 'episode' field stays the newest. Regression test: older-global + newer-broker -> mark -> clear broker -> still considered fired (global kept). B. The flatten_fn-raised branch called mark_sweep_fired unwrapped: if it raised there (disk failure while the sweep also failed), the exception escaped run_once with no audit. Now wrapped for symmetry with the main path — both failures are logged and audited, run_once completes with the halted outcome. Regression test monkeypatches the runner's mark_sweep_fired to raise alongside a raising flatten_fn. 90 live tests pass; ruff clean.
Re-review found a race in the claim window: the halt episode was resolved TWICE — once for claim_sweep at sweep start, once for release_claim in finally. If the operator cleared + re-tripped the halt mid-sweep (ep1 -> ep2), the finally resolved the NEWER episode and deleted EP2's claim — even a different process's — unprotecting that process's concurrent sweep of ep2 (duplicate-close window). The episode is now resolved once, before claiming, and both the claim and its release bind to it. A later episode's claim file is never touched by an older sweep's release. Regression test: mid-sweep clear + re-trip + second-process claim of ep2 -> the ep1 owner's release leaves ep2's claim intact (mutated to the old double-resolution, the test fails: ep2's claim is deleted). 92 live tests pass; ruff clean.
…not per tick #1244 makes the preemptive halt sweep re-check on every halted tick when it attempted no broker write — correct, so a resting order appearing after the first sweep is still cancelled. Its audit record repeated with it: measured on a flat halted book, five ticks went from 5 audit records to 10, one extra per tick forever. At a one-minute tick a channel left halted overnight appends ~1440 identical records to the hash-chained ledger. Two fixes, both keeping the re-check policy: - The record is written once per (episode, condition) per runner; every occurrence is still logged. - A flat book is audited as halt_tripped/blocked, not breach/error. Auditing "no open orders or positions to act on" as a breach made a clean outcome indistinguishable from a real one. Also corrects _halted_result's docstring, which still claimed the sweep runs at most once per runner lifetime.
|
Merged. This is the split I asked for on #1206 and both behaviours reproduce on main exactly as described, so thank you for separating them. Reviewed in detail and I agree with the reasoning on all four of the hard parts: resolving the episode once so the One follow-up pushed on top (
One extra record per tick, forever. At a one-minute tick a channel left halted overnight appends ~1440 of them to the hash-chained ledger. It was also written as The fix keeps the re-check policy unchanged: |
… suffix Reviewer finding: the yfinance fallback inferred the quote currency from the .L ticker suffix and ÷100'd EVERY LSE line. But .L is not uniformly GBp — VUSA.L prices in GBP, VUSD.L in USD (Yahoo history_metadata, checked 2026-08-29): a backtest through the yfinance fallback would scale every absolute quantity (initial-capital sizing, lot rounding, min-notional checks, cross-market composite weights) while returns survive. The direct-Yahoo path already passes the chart meta's real currency into scale_pence_to_currency; the yfinance path now reads the same field via Ticker.history_metadata['currency'] and scales ONLY on GBp/p. A missing currency fails closed: no scale (never assume pence from the suffix). Rebased onto origin/main (e90b6c6, merged HKUDS#1244 era). Tests: GBP-quoted .L (VUSA.L) and USD-quoted .L (VUSD.L) pass through unscaled; absent currency unscaled (fail-closed); GBp still ÷100s (VOD.L).
HOLD review resolved (977b038 — latch-coherence races)
mark_sweep_firedre-read the sentinels after the sweep; a clear + re-trip mid-sweep (ep1→ep2) recorded ep2 as fired though no ep2 sweep ran → next runner skipped it (reproduced). Now: one coherenthalt_snapshot()captured before claiming — claim, already-fired check, marks and release all bind to it;mark_sweep_fired(broker, episodes)never re-reads.FLATTEN_FIRED-<sha16>, independentO_CREAT|O_EXCLatomic creates) — no shared record exists, so no inter-process lock is required and lost updates are impossible by construction.test_mid_sweep_retrip_records_only_swept_episode,test_concurrent_latch_updates_both_episodes_recorded(both mutated-fail / fixed-pass).Re-review race fix (1c9c41a)
Claim-release episode bind: the halt episode was resolved twice (claim time + release time in
finally). A mid-sweep clear + re-trip (ep1 → ep2) made the release delete ep2's claim — even a different process's — unprotecting that process's concurrent sweep (duplicate-close window). Resolved once; regressiontest_mid_sweep_retrip_does_not_release_new_episode_claim(fails against old double-resolution).Review residuals resolved (a4f7459 — non-blocking A & B)
mark_sweep_firednow latches every currently tripped sentinel identity (broker + global, via a shared_sentinel_identityhelper), not just the newest. An older global + newer broker halt: clearing the broker one no longer re-fires the sweep while the global remains tripped. Regression:test_mark_records_both_active_episodes_older_global_newer_broker.mark_sweep_fired— a latch write failure during a sweep failure is logged + audited and never escapesrun_once. Regression:test_double_failure_latch_write_does_not_escape_run_once.90 live tests pass; ruff clean.
Safety hardening (f81cb72 — independent review findings)
sweep_already_fired→ broker writes → durable marker). Two runner processes could both pass the check and duplicate closes. The whole window now runs under an exclusive disk claim (FLATTEN_CLAIM,O_CREAT|O_EXCL); a held claim (other runner active, or a crash mid-sweep) means the episode's outcome is unknowable — never re-sweep, audit for operator resolution, HALT stays tripped. Claim released infinallyon every path.mark_sweep_firedfs error after a broker write is now logged + audited explicitly ("restart may replay; verify broker state"); the claim protocol also bounds the window in which it can occur.{"status":"ok","data":{"status":"error"|"ok":false}}(transport OK, broker rejected) used to be recorded as accepted — order stays live while the sweep latches. Both nested shapes now detected for cancel and flatten.Claim-recovery self-review (episode-keyed): a crash leaving an orphan claim used to block every future episode of the broker (a new trip would see
claim_sweepfail and skip the kill action forever — reproduced). Claims are now episode-keyed (FLATTEN_CLAIM-<sha256>), so an orphan claim only blocks its own episode (the correct unknowable-outcome case) and never a later trip.Tests: claim exclusivity + release, claim-held runner skip, orphan-claim-does-not-block-future-episode regression, claim release on success/read-failure, nested-error rejection for both phases. 89 live tests pass; ruff clean.
Two behaviours being fixed (one per commit, tests each)
Reproduced on current main:
Broker read-error envelopes are iterated as mappings.
MCPServerAdapter.call_toolreturns{"status": "error", ...}instead of raising. fix(live): fail closed on broker error envelopes during the halt sweep #1232 taught the submit responses about envelopes; the sweep's reads (read_open_orders/read_positions) never learned —AttributeError: 'str' object has no attribute 'get'kills the kill-switch action for that episode.Fix (
823fc69b):_read_broker_statevalidates every read before iteration — error envelope, exception, or non-list result → structuredreport["errors"]item (read_open_orders/read_positions), phase aborts with no side effects. Non-dict entries are skipped with an error. Successful adapter responses unwrap with pinned per-read keys (data.orders/data.positions; bare-listdataaccepted) — wrapper dicts without the pinned key are rejected (metadata never becomes fake records).mark_sweep_firedruns beforeflatten_and_cancel, so a sweep whose reads both fail still latches — the kill switch's action is silently skipped for that episode; neither a later tick nor a restart retries it.Fix (
ebf5e189):flatten_and_cancelreportsside_effects_attempted(set the moment any cancel/close submit is tried). The runner latches iff any broker write was attempted (even when another read phase failed), keeps retrying only when nothing was attempted, and latches conservatively on a raising sweep. Reads are also normalized at thelive_routes._readboundary (error envelopes raise; ok payloads unwrap), protecting the shared reconcile path from the same envelope class. Related, same commit:_halt_episodenow ranks broker vs global sentinels by explicittripped_at(mtime ties within one filesystem timestamp quantum let a stale per-broker latch suppress a newer global HALT —halt_flag_settreats the global as authoritative), and the latch record accumulates every fired episode.Tests