Conversation
…, async sells Trailing distance now floors at max(min_trail_ticks, spread, pct of gain) so the stop can never sit inside one tick; the lock arms at +30 percent and take_profit ships disabled so the trail is the primary exit. Marks come only from depth-qualified bids (no mid fallback), peaks update from the book sampler, and live sells run off the event loop with an in-flight registry so settlement/reconciliation cannot double-close.
…dwork quantize_size follows the venue size-decimals rule and never zeroes a whole-share position at 3-decimal prices; sub-share remainders skip and stay open for settlement instead of being written off at zero. Paper and live share one sizing helper, buys abort when the CTF baseline read fails, the write-behind writer counts drops and drains on stop, clob_patch gets tests, manual closes respect the closing registry, and entries persist p_model/confidence/edge behind a calibration report with an edge-sizing knob that ships disabled.
Mutating routes require X-OpenPoly-Token when configured; empty or unresolvable tokens fail closed and live mode is refused (and demoted at startup) without a usable token, even when runtime.json is unwritable. A schema_version table replaces the hand-rolled PRAGMA checks, order book snapshots get a composite index plus an hourly retention prune wired from the canvas config, credential fragments leave INFO logs, and carried review follow-ups land (partial-fill bookkeeping, heat-cap fallback, reconciliation dust filter, calibration sample hygiene).
vitest covers the canvas store and template IO (blocking in CI); the static catalog mirrors the real section schemas and close-all reports partial fills honestly; mutating requests attach the API token from a new settings panel. The backend refuses cross-origin writes via Sec-Fetch-Site/Origin authority checks (vite proxy pins changeOrigin:false to stay same-authority), runtime monitors get architecture docs and a shared TickLoopMonitor base, and mypy plus pre-commit join the toolchain non-blocking.
The review caught six wiring bugs whose units were already well tested: switching to live built a live executor and discarded it (every trade skipped live_not_ready until restart), the lifespan never called bootstrap_peaks (each restart forgot every run-up), the manual close routes ran the blocking sell inline on the event loop, canvas edits to the retention window were persisted and ignored, spread was measured from the raw level-1 bid instead of the depth-guarded mark, and a reconciled close reset the consecutive-loss brake. The rest are smaller: per-position isolation in close-all, a bare recorded_at index for the retention DELETE, a stop check between prune batches, auth failures no longer render as "backend unreachable", and the secret-ref scheme list has one owner again. Each of the six ships with a regression test proven to fail when its fix is reverted — these survived precisely because unit coverage does not exercise wiring.
CI text predated the frontend test job and the informational mypy / ruff-format steps. Adds the two conventions this work introduced that an agent would otherwise miss: a schema change needs a MIGRATIONS entry (create_all only covers fresh databases) and every mutating route declares the API-token dependency.
The live executor posts crossing GTC limit orders at the level-1 price. When nothing crossed it returned live_no_match and left the order resting on the book at a price that was already stale, where it could fill later with no ledger row — the same orphan incident the partial-fill cancel was written for, one branch over. Per the current CLOB docs there is no status that means "not placed": `unmatched` rests on the book after the delay window, `delayed` cannot be cancelled while pending, and every cancel answers 200 with `canceled` / `not_canceled`, which the SDK hands back unread. Every answered order short of a full fill is now settled. The cancel is retried inside a bounded budget (8 s of wall clock, at most 16 attempts) across the matching-delay window; the cancel body is read; a refused cancel is checked against the order itself, and only a full size_matched or a cancelled status ends the loop — MATCHED alone does not, because a partial fill reports it too. The largest matched size seen is never discarded, one fresh read follows the loop unless a read already proved the order terminal, and the CTF balance delta stands in when that read fails. A fill that raced the cancel is recorded at the limit price, the conservative bound. ExecResult carries resting_order_id so callers can back off instead of posting a second order on top of one that may still rest (the exit-monitor consumer and the startup sweep follow separately). BUY persistence gets the same bounded retry as SELL, and non-dict responses, unparseable amounts and a missing USDC amount no longer escape the never-raise contract or book a zero price. New skip reasons: live_cancel_failed, live_fill_unknown, open_persist_failed. Judgment Day approved the change after one correction round (wall-clock bound, max-seen fill, MATCHED-is-not-terminal). Tracked follow-ups: per-asset cancel after a lost response, resting-order sweep at live start, explicit SDK timeout, and the live_fill_unknown guard when the only in-loop read was a stale zero.
The runtime-monitors doc and the Phase 4 changelog entry still said that `bootstrap_peaks` was not wired and that trailing-stop peaks reset on every restart. The same-day code-review follow-up (b52f115) wired it into the lifespan between configure() and start(), with a regression test pinning the call, so a lock that armed on an earlier run-up now survives a restart for as long as the snapshot retention window covers the position. Both texts now say so, with the retention caveat spelled out. The strategy changelog also gains the entry the fill-or-cancel change deserved: an order that did not cross is cancelled rather than left resting, the cancel response is read, MATCHED alone is not "nothing rests", a fill that raced the cancel is booked at the limit price, and the executor reports live_cancel_failed / live_fill_unknown instead of claiming a clean miss.
Without a .python-version, setup-uv resolves the newest CPython that satisfies requires-python at each CI run, so a new interpreter release can break the build through missing wheels (torch via sentence-transformers) with no change in the repo. Pin 3.13, which is what the local venv already runs, so nothing rebuilds locally and CI matches it; whether to move the project to 3.12, its declared target, or stay on 3.13 is still an open decision. .atl/ (the personal skills registry cache) and .coverage (pytest-cov data) join the ignore list.
The format check ran with continue-on-error because a newer ruff could reformat code under us and block merges for reasons unrelated to the change. That rationale no longer holds: ruff is pinned to an exact version in pyproject, so the rules cannot move without a deliberate bump. The tree is already clean, so promoting the step costs nothing today and stops drift from creeping back in. AGENTS.md lists the gates as CI actually enforces them.
The workflows pin every action to an exact version and both lockfiles pin every dependency, which is right, but nothing was moving them. Weekly grouped minor/patch updates for the three ecosystems keep the pins current without a flood of PRs; majors still arrive one at a time.
mypy shipped as a reporting step because it was introduced onto a codebase that never had it. The 14 errors it reported were all type hygiene, not behavior, so clearing them costs nothing and closes the loop: the gate can block now, and the config keeps its deliberate leniency about the third-party SDKs that ship no stubs. The one that was worth the walk is the kill-switch check. It reads a slice of closed positions where closed_at and realized_pnl are Optional on the shared record, and the filter that guarantees they are set could not survive the comprehension, so the arithmetic in all three brakes was unprovable. A small NamedTuple built by the same filter carries the narrowing forward and leaves the brakes byte-identical. Elsewhere: the pipeline hooks are typed by what the managers do with the return value, which is nothing; the LLM tool definitions are typed with the SDK's own ToolParam instead of a dict plus a cast, so a malformed tool is a type error rather than a 400; the query helper takes a Mapping so its callers stop annotating dict literals one by one; and the wallet balance cache holds the response model rather than dumping and re-validating it on every hit. CONTRIBUTING.md and the pre-commit config still told contributors that format and mypy could not fail a PR, which stopped being true one commit ago; both now list the real gates.
The note added two commits ago said a position older than the snapshot retention window re-seeds its peak at the first mark observed after a restart. That is not what happens: the prune deletes snapshot rows by age alone, and bootstrap_peaks takes the max depth-guarded mark over every row that survives for the position, so an old position still rebuilds from the recent window and only forgets a run-up older than it. Entry price is the fallback when no snapshot survives at all. The distinction matters to anyone sizing the retention window from that paragraph. The exit monitor's own comment about partial sells still described the fill model as IOC, which the fill-or-cancel rewrite replaced.
Weekly grouped minor and patch updates are right for the ordinary dependencies and wrong for three of them. py-clob-client-v2 is pinned to a pre-release, places real orders, and has its HTTP layer monkey-patched by clob_patch, so a bump that rides in a group merges on a green suite that proves nothing about the venue — every executor test runs against a fake. Ruff and mypy are the pinned tools behind two blocking gates; a grouped bump can reformat the tree or tighten a check, which is exactly the review that should not be buried among a dozen unrelated version lines. The SDK is now ignored here and moves only through a deliberate upgrade with a paper run; ruff and mypy still get PRs, just their own.
The settle path trusted the venue's own account of an order more than it should. Both amounts of a POST response were parsed in one try, so a garbage price field zeroed a share count reported alongside it: a response saying all ten shares filled became a cancel against an already-filled order and no ledger row for tokens the wallet was holding. One field of the same body could still crash the executor outright — a transaction-hash list arriving as a bare number is not subscriptable — and a bare string was indexed into, storing its first character as the hash. That body is now parsed once into a view whose every field is validated by construction, so no shape of it reaches the ledger or an exception. Amounts that will not parse are unknown rather than zero, and that now includes NaN and the infinities: they convert without raising, and then make every comparison downstream false, so a share count nothing could read was reported as a clean miss and an infinite price was persisted as a cost basis. A share count larger than the order is discarded rather than believed — clamping it looked tidier but booked a full-size position against an empty wallet, which blocks re-entry and can never be closed. The order read is clamped instead, since it is what the settle trusts once the response is discarded, and an unusable size there makes the whole read a failed read. The cancel body gets the same treatment: an acknowledgement must be a list holding our id, not a string that merely contains it, and an unexpected shape is a refusal rather than an exception, because raising abandons the order on the book. An id that is a whole number is sent rather than dropped, since not cancelling is the worse failure. Finally, the resting-order alert is cleared only by evidence about this order. The wallet balance cannot say which order moved it, so an earlier order's remainder filling during the same poll could have silenced the alert for one still on the book. A false alert costs a look at reconciliation; a suppressed one costs the position. Judgment Day approved this after one correction round; the follow-ups it listed (an explicit transport timeout, rejections told apart from lost responses, the never-raise contract made structural) are tracked separately.
The settle had been patched a round at a time until it carried seven interacting variables and no stated rule for which account of an order wins. It contradicted itself in both directions. A garbage order read of 999 shares on a ten-share order was booked down to ten and recorded as a full fill — a position against an empty wallet that blocks re-entry and can never be sold, which is precisely what discarding the same nonsense from the response body was written to prevent. One line over, a read of zero taken while the order was still live was trusted over a wallet that could see the shares. Every quantity the executor learns is now evidence, and the rules are written down where the code applies them. A quantity above the order size is not a quantity: it is a wrong-scale field, discarded from any source rather than clamped into a plausible-looking lie. A reading of the order taken once the order can no longer fill is the venue's own account and decides alone — but it never resolves below an earlier reading of that same order, because a count of what one order matched only grows, and a smaller later answer is the data API lagging the matching engine. The response's immediate cross, a reading taken while the order could still fill, and the wallet delta are lower bounds. Whether anything may still rest is a separate question, and the wallet never votes on it: it is a wallet-wide delta attributed to no order, so another order's remainder settling in the same window is indistinguishable from this one filling. The wallet is the single source clamped rather than discarded, and both places that ask it now ask the same function. An order for exactly this size was sent, so a delta above it still means at least that much was ours; discarding it is the worse error, because the wallet is the last source to speak and a larger delta would otherwise book less than a smaller one. Prices get the same treatment. A quotient outside the range a token can trade in is not a price, and an order is refused outright rather than posted at a level-1 price the venue could not have produced — the band lives beside the notional minimum, with the other venue rules. Judgment Day and two review passes drove this; the trade-off they weighed is that a clean no-fill cancel now consults the wallet before accepting the zero, which costs a poll on a common path and is the only way to see a fill that raced the cancel. What they found and this does not answer is tracked: paper still applies fewer gates than live, the band is wider than the SDK's own, and the resting-order signal still has no reader.
MAX_TOKEN_PRICE was 1.0, which is not the venue's actual rule. py-clob-client-v2 requires tick_size <= price <= 1 - tick_size, so the finest tick (0.0001) puts the true ceiling at 0.9999, not 1.0. A price like 0.99995 sat inside the old band and would have been signed into a live order the SDK's own builder refuses. The predicate that checks this band also lived in the wrong place: it was a private function inside live_executor.py even though its constants already lived in sizing.py, the module both executors are supposed to size through so paper never simulates a fill live would reject. The result was that LiveExecutor enforced the band and PaperExecutor did not enforce it at all — it booked book.asks[0][0] or book.bids[0][0] straight as a cost basis with no bound. A companion fix on this branch already stops NaN and out-of-(0,1) levels from reaching that code, but a legal-looking level like 0.00005 or 0.99998 still got through untouched. in_price_band now lives in sizing.py, public, and both executors call it: LiveExecutor imports it instead of defining its own copy, and PaperExecutor checks it right after reading the level-1 price, before sizing, mirroring the order live already used. Three log format specifiers that printed MAX_TOKEN_PRICE with %.1f (correct when the value was 1.0, misleading now) were switched to %.4f. Not addressed here: sourcing tick size per-market instead of using the one finest-tick band for every market is out of scope for this change.
.python-version pins 3.13, the interpreter uv run actually executes on, but requires-python and the ruff/mypy targets still said 3.12 — four blocking gates were analysing a different language version than the one running them. mypy's own version wasn't pinned either, just a floor (>=1.14), unlike the exact ruff==0.15.13 beside it, even though CONTRIBUTING.md and dependabot.yml already claimed both tools carry exact pins. requires-python is now >=3.13, ruff's target-version and mypy's python_version are both py313/3.13, and mypy is pinned to 2.3.1 — the version uv.lock already resolved under the >=1.14 floor — so the docs' claim is now true instead of aspirational. uv lock picked up the tightened requires-python, which drops the now-irrelevant cp312 wheel entries and markers from the lockfile. The README badge advertised 3.12 too; that's fixed. ruff check, ruff format --check, mypy, and the full pytest suite all stay green under python 3.13.7.
A Polymarket price is a probability: nothing on the CLOB rests outside (0, 1), and a size is never zero or negative. parse_clob_book trusted the raw payload past that — it dropped a level only when price or size failed to parse at all, not when the parsed value was garbage. A 1.5 bid sorted to the front of the book as the "best" price, silently making a position unsellable; a 0.0 ask survived into any caller that divides notional by it, a ZeroDivisionError two frames from the network response. NaN and the infinities parsed as valid floats too and propagated the same way. _to_float now rejects non-finite values, and _book_levels drops any level whose price is not strictly between 0 and 1 or whose size is not positive. This is the upstream fix: it makes a wrong-scale book fail at the one place it enters the system, instead of at whichever downstream caller happens to divide or sort by it first — including the paper executor, which books its cost basis straight off the book with no check of its own.
Two gaps in the same code path. First, the SDK's module-level httpx client carries no timeout of its own — ClobClient.__init__ takes no timeout parameter, so a single request was bounded only by httpx's current default, which the settle-loop budget comment in live_executor.py already flagged as the one thing it could not account for. clob_patch now replaces that client with one pinned to httpx.Timeout(5.0) — httpx's own current default, so this changes nothing about actual behavior today, only makes the number explicit and immune to a future httpx version silently changing it. Second, execute_buy and execute_sell wrapped self._post_order in a bare except that always ran the lost-response recovery path — polling the wallet's CTF balance to guess whether an order landed despite a lost reply. But py_clob_client_v2 already distinguishes the two cases: PolyApiException carries a real status_code when the venue answered with a non-200 (bad precision, min-size, closed-only mode, ...), and status_code is None only for a genuine transport failure. A definitive rejection was being treated as if the response had gone missing — polling a balance that could never have moved, and discarding the venue's own rejection reason in the process. Both call sites now check status_code first: a real rejection logs a warning with the venue's status and message and returns live_rejected: immediately, with no balance poll; everything else (status_code is None, or not a PolyApiException at all) falls through to the existing lost-response path unchanged. PolyApiException is re-exported from clob_patch alongside the other SDK types so callers never import the SDK's exceptions module directly.
…as ok
_persist_irreversible retried the ledger write on any Exception, but only
sqlalchemy.exc.OperationalError (SQLite lock contention) is actually
transient. IntegrityError (a genuine duplicate against the partial unique
index) and ValueError (position not found / already closed / not open) fail
identically on every attempt, so retrying them just burns the full
_PERSIST_ATTEMPTS x _PERSIST_SLEEP budget and logs a misleading "retrying"
message for something no retry can fix. The retry is now narrowed to
OperationalError; everything else propagates on the first attempt with no
sleep, straight to the caller's existing CRITICAL log. The wait-budget
comment above _PERSIST_ATTEMPTS undercounted this term too: PRAGMA
busy_timeout=5000 (db/engine.py) means a single locked-DB call can already
block up to 5s inside SQLite before OperationalError is even raised, so the
worst case for that error class is closer to 5 x (5s + 0.5s) ~= 27.5s, not
the "~2s" the comment implied; the comment now says so and notes that a
permanent failure now costs one call and no sleep.
Separately, orchestrator._run_entry lost a BUY's ledger-write failure on the
way to the entry log. execute_buy never raises for this case (a deliberate
never-raise contract) - it returns skip("open_persist_failed:<...>") after
an on-chain fill already succeeded, so the section's own verdict stayed
"ok" and the except Exception branch below it never fired. A wallet now
holding tokens no ledger row manages was indistinguishable in verdict from
a routine, benign skip like dust or price_out_of_band. _run_entry now
checks for the open_persist_failed: prefix specifically and flips that one
case to verdict="error" with an explanatory error message; every other
skip reason keeps verdict="ok" unchanged, since declining to open a
position is normal and most skips are not failures.
Four bounded defects in the same call sites. The success check treated
resp.get("success") as the whole answer: an absent key (some response
variants omit it) made not None true and returned live_rejected
immediately, abandoning a possibly-resting order with no cancel attempted;
a string "false" made not "false" false and sailed straight into
_resolve_fill as though the order had succeeded. Both call sites now go
through a new _order_was_rejected helper that returns True only when
"success" is present and is either the boolean False or a string equal,
case-insensitively and stripped, to "false" - everything else, including
an absent key, proceeds to settle.
_ORDER_DONE_STATUSES was missing EXPIRED and REJECTED, two terminal
statuses the venue's order lifecycle already documents alongside
CANCELED/CANCELLED; without them the settle loop kept burning its
retry/cancel budget on an order the venue had already reported dead.
_read_order also treated every get_order failure alike, including a 404,
which for this endpoint means the order aged out of the venue's records
and is definitely gone rather than a transient read worth retrying. It now
catches PolyApiException separately: a 404 returns the synthetic status
PURGED (added to _ORDER_DONE_STATUSES so add_read treats it as terminal
with no other change needed), any other status code falls through to the
existing warn-and-return-None behavior.
Every function that branches on side treated anything other than exactly
"buy" as "sell" via an if/else with no final raise, so a caller bug (a
typo, or passing intent.side, which is a real "yes"/"no" domain that
already exists in this codebase) would silently book the wrong amount as
shares and the wrong price edge as conservative instead of failing loudly.
_bookable_price, _resolve_fill, and _fill_from_balance now raise
ValueError up front for anything other than "buy" or "sell"; both existing
call sites already pass correct literals, so this only guards against a
future internal mistake.
test_live_executor.py's no_sleep fixture patched attributes on le_mod.time,
but le_mod.time is the real, process-wide time module object (Python caches
modules in sys.modules, so there is only ever one). Any other code running
while a test held that patch — a background thread, an in-process xdist
worker, coverage.py's own instrumentation — saw the fake sleep or, worse for
test_settle_stops_at_the_wall_clock_deadline, the fake stepping monotonic
clock, whose shared mutable counter could be consumed by an incidental call
from outside the test and throw off the wall-clock-deadline assertion. The
fixture now rebinds live_executor.py's own module-level time name to a fresh
types.SimpleNamespace instead, leaving sys.modules["time"] untouched; the
namespace's monotonic defaults to the real time.monotonic so a test that
never asked for a fake clock still gets real elapsed time. The wall-clock
test's own monkeypatch.setattr(le_mod.time, "monotonic", ...) line needed no
change — it now sets an attribute on the fake namespace instead of the real
module, composing automatically once no_sleep hands out the namespace first.
test_execution_sizing.py's _NoopClob.cancel_order returned None
unconditionally. LiveExecutor's settle path treats any non-dict cancel
response as a refusal, so the first partial-fill test against this fake
would burn the entire cancel-retry budget treating every attempt as refused
— and, since this file had no sleep-scoping fixture of its own, would do it
through real time.sleep calls. cancel_order now returns a realistic
acknowledged-cancel body ({"canceled": [payload.orderID], "not_canceled":
{}}), and the file gains the same scoped no_sleep fixture as
test_live_executor.py so any retry loop a test drives through LiveExecutor
never sleeps for real. Kept as a duplicate rather than moved into
conftest.py: consolidating would make every test in the suite pay for an
import and monkeypatch of live_executor's time name, including tests with
no relationship to it, for no benefit over the two small, self-contained
copies.
test_execute_sell_runs_off_the_event_loop proved the slow sell ran in a worker thread by counting asyncio heartbeats (10ms apart) during a 300ms blocking call and asserting at least 10 landed. Under CPU load or scheduler jitter, a starved heartbeat task can log fewer ticks even though the event loop never actually stalled, which is the flakiness prior review runs reported (about 3 in 10 full-suite runs). The count carried no information the test actually needed: what matters is not how many heartbeats fit in the window, only whether the loop kept running at all while the sell slept. The test now races a single heartbeat tick against the slow call itself with asyncio.wait(..., return_when=FIRST_COMPLETED): the tick only wins if the event loop was genuinely blocked for the whole 300ms, a margin no ordinary scheduling jitter can close, and it still fails the same way a regression would — offloading removed, the blocking call finishes long before the loop ever gets to run the heartbeat's timer. test_ws_client_reconnects_after_drop and the buffer-length assertion beside it (both in test_news_ws_client.py) were also named in the review as occasionally flaky, but they already poll toward a generous 3s ceiling rather than counting within a fixed window; their real sockets and real reconnect backoff are the more likely source of load sensitivity, and replacing that with a fake transport is a larger change than this pass should force. Left alone.
…erID is not settleable
A Judgment Day correction round on the last three execution commits. Both
judges independently found the same three places where a signal was read as
more certain than it is.
The rejection check around the order post asked only whether
PolyApiException.status_code was not None, but that is true of every non-200
the venue ever answers, 502/503/504 included. The pinned SDK's own
_is_transient_error classes 500 <= code < 600 as transient precisely because
a 5xx is not proof the order was never placed: the matching engine can have
accepted and matched it and only the response leg failed. Short-circuiting
past the CTF balance-confirm there leaves a real on-chain fill with no ledger
row, the untracked position the pre-order baseline read exists to prevent.
Both call sites now require a 4xx to call it a definitive rejection;
everything else, 5xx included, falls through to the balance-confirm unchanged.
_read_order treated a 404 from GET /data/order as the synthetic terminal
status PURGED. But this file documents at length that the venue's data API
lags the matching engine, so a just-placed order — especially one still inside
the non-cancellable matching-delay window — can 404 simply because it has not
been indexed yet, which is indistinguishable here from an order that really
aged out. Believing it ended the cancel-retry loop after a single attempt and
dropped the RESTING ORDER ALERT for an order that may still have been live on
the book. The 404 branch and the PURGED entry in _ORDER_DONE_STATUSES are
gone; a 404 is a failed read like any other and the existing attempt and
wall-clock budget decides when to stop. EXPIRED and REJECTED stay: those are
real statuses returned in a successful read, not inferred from an absent one.
_order_was_rejected returned "proceed to settle" for any body without a
"success" key. That is right for the case it was written for — some variants
omit the key on an order that was genuinely placed, with an orderID and
amounts to settle on — and wrong for an error-shaped body such as
{"error": "..."}, which has neither. That one flowed into the settle with a
None order id, logged a false RESTING ORDER ALERT, spent the balance-poll
budget, and could book a fill out of any unrelated balance move in that
window; the module's own notes name an earlier order's remainder settling as
exactly such a move. An absent "success" key now only proceeds when the body
also carries a non-empty orderID. The skip message reads the venue's words
through a small _rejection_reason helper, since an error-shaped body names the
text "error" rather than "errorMsg" and dropping it left the reason saying
nothing.
AGENTS.md still advertised the ruff target as py312; the project was
reconciled to 3.13.
Twelve commits from the 2026-09-05/06 audit: settle-loop fill accounting (fill-or-cancel integrity, malformed-response handling, one rule for what a fill was), order-book ingest validation, a shared price band for both executors, honest transport/rejection handling, scoped persistence retries with a restored error channel, corrected settle signals (success-flag parsing, terminal statuses, side guards), test-hygiene fixes, and version-pin reconciliation to Python 3.13. Judgment Day ran across the full batch at the end; three confirmed defects were fixed and a scoped re-judgment came back clean.
…port, not a real socket test_ws_client_reconnects_after_drop was flagged flaky under CPU load (~3 of 10 full-suite runs) when e86f6a9 fixed its exit-monitor sibling; that commit diagnosed the residual cause as real sockets and real reconnect backoff racing a wall-clock ceiling, and deferred the larger fix of swapping in a fake transport. Rewrites the test to monkeypatch websockets.connect (same mechanism test_on_event_auth_fail_stops_loop already uses) instead of running a local websockets.serve server, and waits on an asyncio.Queue the fake connector fills per attempt instead of polling a counter toward a timeout. No real I/O is left on this path, so the test is bounded only by event-loop scheduling, not real socket/backoff timing. Verified: 30/30 passes under artificial CPU load (6 busy-loop processes), and the test goes red when run_forever's reconnect is sabotaged, confirming it still catches the regression it's meant to catch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ver the backoff branch Two confirmed follow-ups from the full review of 17b9187, which fixed test_ws_client_reconnects_after_drop's CPU-load flakiness but left two gaps: test_on_event_reconnect_attempt_after_drop had the identical real-socket-vs-wall-clock race shape, never caught flaky but latent; and run_forever's exponential-backoff/exception branch (the except ConnectionClosed/WebSocketException/OSError path) had zero coverage anywhere in the file. Extracts the fake-connect scaffolding both reconnect tests shared into _fake_reconnect_twice, migrates test_on_event_reconnect_attempt_after_drop onto it, and adds test_ws_client_backs_off_after_connection_drop: raises directly from a synchronous connect stub (run_forever's try already wraps the connect() call itself, no async-context-manager wrapper needed), asserts the retry waits out the real backoff with no fudge factor (asyncio.wait_for timeouts never fire early), and cross-checks the on_event "disconnected" detail against the clean-close path's. Ran /simplify then /code-review max per house habit before committing; applied the confirmed findings: renamed the sibling test to ..._after_clean_close (it tests a clean close, not a drop — its old name read as symmetric coverage with the new backoff test), narrowed _fake_reconnect_twice's **client_kwargs to an explicit on_event param (the blind forward had no catch-all on the client side and could raise a confusing TypeError on a future typo), gave a bare next() a default so a future regression fails with a clear assertion instead of RuntimeError: coroutine raised StopIteration, unified the file onto one websockets.connect patch spelling, and shrunk the new test's backoff to this file's usual 0.05/0.1. Verified: full suite green, both timing-sensitive tests stable across 8 repeated runs, ruff and mypy clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Opened against the wrong repo by mistake (gh defaulted to the fork parent) — recreating against nahimrgz/OpenPoly instead. |
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
1 participant
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.
Summary
Two confirmed follow-ups from the full 10-angle review of 17b9187 (which fixed
test_ws_client_reconnects_after_drop's CPU-load flakiness):test_on_event_reconnect_attempt_after_drophad the identical real-socket-vs-wall-clock race shape — never caught flaky, but latent. Migrated it onto a shared_fake_reconnect_twicehelper (extracted from the two reconnect tests) instead of a realwebsockets.serveserver.run_forever's exponential-backoff/exception branch (except ConnectionClosed/WebSocketException/OSError) had zero test coverage anywhere in the file. Addedtest_ws_client_backs_off_after_connection_drop: raises directly from a synchronousconnectstub (no async-context-manager wrapper needed —run_forever'stryalready wraps theconnect()call itself), asserts the retry actually waits out the real backoff with no fudge factor (asyncio.wait_fortimeouts never fire early), and cross-checks theon_event"disconnected" detail against the clean-close path's.Ran
/simplifythen/code-review maxper this repo's usual pre-commit habit and applied the confirmed findings:..._after_clean_close— it tests a clean close, not a drop, and its old name read as symmetric coverage with the new backoff test._fake_reconnect_twice's**client_kwargsto an expliciton_eventparam — the blind forward had no catch-all on the client side and could raise a confusingTypeErroron a future typo.next()a default so a future regression fails with a clear assertion instead ofRuntimeError: coroutine raised StopIteration.websockets.connectpatch spelling (dropped a leftoverwsc_modindirection).0.05/0.1(the oldmax_backoffwas provably inert).Test plan
pytest— full suite greenruff check/ruff format --check— cleanmypy(scoped toopenpoly/, per this repo's ownpyproject.toml) — clean🤖 Generated with Claude Code