[keitaj]

Summary

• MarketDataFeed._last_update is only written when a coin receives its first l2Book message, so stale_coins() compared not-yet-ticked coins against the dict default of 0. Since time.monotonic() returns the host's uptime, that comparison is unconditionally true on a long-running host and every coin is reported stale until it ticks.
• WsReconnector._is_healthy only reconnects when all coins are stale — which is precisely the state immediately after subscribing. Combined with _last_check starting at 0.0 (so the first health check runs right away), the WS stack was torn down and rebuilt within milliseconds of every subscribe.
• Fix seeds _last_update when a subscribe succeeds. Observation-only in effect: no change to order placement, no new config parameter, no new dependency.

Changes

ws/market_data_feed.py

• _subscribe_coin seeds _last_update inside the existing lock, on success only.
• stale_coins docstring now states the contract: a successfully subscribed coin goes stale only once max_age has actually elapsed; a coin whose subscribe failed is never seeded and is reported stale immediately.

Two design points worth stating explicitly:

• Seeding rather than filtering. The alternative — skipping unseeded coins inside stale_coins() — would also hide a coin whose subscribe raised, turning a false positive into a false negative. With seeding, "subscribed" and "seeded" coincide, so a failed subscribe surfaces as stale immediately rather than after the threshold.
• setdefault rather than assignment. Assignment would let a re-subscribe of a silent coin move its timestamp forward and make an ongoing stall look fresh. The two forms behave identically in the current call graph (_subscribe_coin is only reached from start() on a fresh instance), so this pins the safer form for any future feed-reuse path.

tests/test_ws_market_data_feed.py — 6 tests: nothing stale right after start(); the production regression under a large uptime; a seeded coin still going stale once max_age elapses; a failed subscribe reported stale immediately; setdefault preserving an old timestamp across a re-subscribe; and seed/tick timestamps sharing one clock.

tests/test_ws_reconnector.py — 2 integration tests that wire a real MarketDataFeed into the bot mock, since every other test in that file stubs stale_coins and so cannot catch a defect in the staleness computation itself. One asserts a fresh subscribe does not reconnect; the other that a genuinely stalled feed still does.

On the tests patching time.monotonic

This is deliberate, not incidental. With a small monotonic value, now - 0 > max_age is false and the defect is invisible — these tests would pass without the fix. That is exactly why the bug only ever manifested on a long-running host, so the regression tests set the clock to a large value explicitly.

Each new assertion was mutation-tested:

Mutation	Result
Remove the seed line	5 tests fail, including both integration tests
setdefault → assignment	1 test fails (the re-subscribe test)

The two tests that pass under both mutations are the ones guarding against over-suppression (real staleness still detected, real stall still reconnects), which is intended.

Scope

This fixes only the spurious reconnect at subscribe time. Separately, logs on a long-running host show all coins going silent for >60s periodically, which the reconnector recovers from correctly; cause unknown and out of scope here. The value of this fix for that investigation is that afterwards every reconnect log line corresponds to a genuine stall, where today the first one is always noise.

Level 1 risk guards untouched.

Test plan

• Unit tests added (6 feed + 2 reconnector integration)
• Each new assertion verified by mutation testing
• flake8 --max-line-length=120 clean on changed files
• pytest tests/ -q passes: 1330 passed
• No regressions — the pre-existing test_stale_coins passes unchanged

🤖 Generated with Claude Code

[keitaj]

Review: seed WS staleness on subscribe

Correct diagnosis and the right fix. The defect is real and the reasoning holds up end to end: _last_update is only written on the first l2Book message, stale_coins() falls back to the dict default of 0, and time.monotonic() is host uptime — so a not-yet-ticked coin is unconditionally stale, which is exactly the all-coins-stale condition _is_healthy reconnects on. Scope is tight (one production line plus tests), no new config, no new dependency, risk guards untouched.

Two choices in the PR deserve to be called out as correct rather than merely acceptable:

• Seeding over filtering. Skipping unseeded coins inside stale_coins() would read as the smaller change but would also mask a coin whose subscribe raised. The PR picks the option that keeps a failed subscription detectable, and test_failed_subscribe_is_reported_stale_immediately pins it. That is the difference between fixing a false positive and converting it into a false negative.
• Patching time.monotonic in the tests. Without it now - 0 > max_age is false and the tests pass against the unfixed code. The PR states this and the mutation results confirm it — removing the seed fails five tests, including the two that wire a real MarketDataFeed into the reconnector instead of stubbing stale_coins. Those two matter: every other test in test_ws_reconnector.py stubs the staleness computation and so could never have caught this.

I also checked that real detection is not blunted — test_seeded_coin_goes_stale_once_max_age_elapses and test_genuinely_stale_feed_still_reconnects cover both directions, and the pre-existing test_stale_coins passes unchanged.

Two nits, neither blocking.

---

1. The seed sits inside _lock, which does not protect _last_update (nit)

ws/market_data_feed.py — _subscribe_coin:

with self._lock:
    self._subscription_ids[coin] = sub_id
    self._last_update.setdefault(coin, time.monotonic())

The only other writer, _on_l2_update, updates the same dict without holding the lock:

self._last_update[coin] = time.monotonic()   # WS thread, no lock

So the lock here is incidental — it is held for _subscription_ids, and placing the seed inside it suggests a guarantee for _last_update that the class does not actually provide.

Correctness is unaffected either way: both operations are single GIL-atomic dict writes, and setdefault cannot clobber a concurrent real timestamp. I traced both interleavings — the WS thread writing its timestamp before or after the seed — and the real value wins in both.

Suggestion: keep it where it is (grouping it with the subscription bookkeeping is reasonable) but say so in the comment, e.g. "inside the existing lock for _subscription_ids; _last_update itself is written lock-free from the WS thread and relies on single-op atomicity". Otherwise a future reader may add locking to _on_l2_update believing it restores a symmetry that was never there.

---

2. The same idiom exists in the oracle guard with the opposite, deliberate intent (nit)

ws/oracle_divergence_guard.py L123, L217, L364 use the identical shape:

last_change = self._last_oracle_change.get(coin, 0.0)
if prev is None or (now - last_change) > self.stale_ttl_seconds:

Here the 0.0 default is intentional, and the surrounding comment says so: "First sighting or stale recovery: re-baseline the value but do NOT arm the guard on a mere snapshot. A frozen, market-closed oracle must not be treated as fresh". The default makes that path fail-safe (guard stays disarmed), where in market_data_feed.py it caused an unwanted action (teardown). Same code shape, opposite correct behaviour.

Nothing to change in the guard. The risk is to the next reader: having fixed the feed, a reasonable person may well "fix" the guard symmetrically and silently break market-close detection.

Suggestion: one line in the new comment noting that the oracle guard .get(coin, 0.0) default is deliberate and must not be seeded the same way.

---

Verdict: LGTM

No blockers, no should-fix items. CI is green on all six checks, the diff is 172 lines across one production file and two test files, and every new assertion has a verified mutation target. Both nits are comment-only and can be folded in without a re-review.

Scope is correctly limited to the subscribe-time false positive. The periodic all-coins-silent stalls that the reconnector recovers from are a separate question and are properly left out; the useful side effect of this fix is that afterwards every reconnect log line corresponds to a genuine stall, where today the first one per session is always noise.