Skip to content

test(integration): poll subaccount fixture for eventual consistency#128

Merged
TexasCoding merged 1 commit into
mainfrom
fix/integration-subaccount-cleanup
May 17, 2026
Merged

test(integration): poll subaccount fixture for eventual consistency#128
TexasCoding merged 1 commit into
mainfrom
fix/integration-subaccount-cleanup

Conversation

@TexasCoding

Copy link
Copy Markdown
Owner

Summary

`POST /portfolio/subaccounts` returns the new `subaccount_number` immediately, but `GET /portfolio/subaccounts/balances` lags by ~1.0-1.5s before the new account appears in the list. The `ephemeral_subaccount` fixture asserted visibility immediately after create, so it failed when the listing hadn't propagated yet.

Probe evidence:

```
created: 23
poll 1 (0.0s): created 23 in list? False
poll 2 (0.5s): created 23 in list? False
poll 3 (1.0s): created 23 in list? True
```

Replaced the immediate return with a 10s poll loop on `list_balances` (0.5s interval). Soft-skips with a clear message if the new number never appears within the budget.

Verification

  • `tests/integration/test_subaccounts.py` — 13 passed, 1 skipped (skip is the unrelated `integration_real_api_only` netting endpoint)
  • Previously: `test_list_balances_reflects_ephemeral_subaccount` was the only failure here

Progress on #124

Closes 1 of 11. Running tally: A (#126=4) + B (#127=1) + E (this=1) = 6 of 11. Remaining 5 are the `not_found` cluster in test_order_groups and test_orders (likely demo limitations or test setup issue — investigating in the next PR).

Refs #124

Test plan

  • Local integration suite for test_subaccounts.py passes
  • CI green

POST /portfolio/subaccounts returns the new subaccount_number immediately,
but GET /portfolio/subaccounts/balances lags by ~1.0-1.5s before the new
account appears in the list. The ephemeral_subaccount fixture asserted
the number was visible immediately after create, so it failed when the
list hadn't propagated yet.

Replaced the immediate return with a 10s poll loop on list_balances
(0.5s interval). Soft-skips with a clear message if the new number never
appears, so future demo-side issues don't masquerade as test bugs.

Closes 1 of 11 integration-test failures (#124).
Running tally: A (#126=4) + B (#127=1) + E (this=1) = 6 of 11.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 17, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR fixes a legitimate eventual-consistency race in the ephemeral_subaccount session-scoped fixture. After POST /portfolio/subaccounts, the balances listing lags by ~1.0–1.5s on demo, causing test_list_balances_reflects_ephemeral_subaccount to fail sporadically. The fix polls list_balances for up to 10s (0.5s interval) and soft-skips if the account never surfaces.


✅ What's done well

  • Clock choice is correct. time.monotonic() is the right choice here — it's unaffected by system clock adjustments, unlike time.time().
  • Surgical change. Only the fixture body is touched. No unrelated cleanup.
  • pytest.skip semantics are correct. Called from a session-scoped fixture, it propagates to all dependent tests — appropriate for a demo-side propagation failure that's outside the test's control.
  • Field access is consistent. balances.subaccount_balances matches the field name used in test_list_balances and test_list_balances_reflects_ephemeral_subaccount.
  • Import ordering. import time is inserted alphabetically between logging and uuid. ✓
  • 10s budget is defensively generous. Observed lag is 1.0–1.5s; 20 attempts gives a wide margin for slow CI environments.
  • Docstring update is accurate and explains the why, not just the what.

Suggestions

1. Add a log line on successful poll (minor)

The fixture already logs at create time. Logging when the account becomes visible makes CI output easier to parse when investigating future failures:

if any(b.subaccount_number == created for b in balances.subaccount_balances):
    elapsed = time.monotonic() - (deadline - 10.0)
    logger.info("Subaccount %s visible in list_balances after %.1fs", created, elapsed)
    return created

Not a blocker — just adds observability in the same style as the existing logger.info.

2. First poll always misses (cosmetic, not a bug)

Given the ~1.0–1.5s observed lag, the first two polls (at t≈0s and t≈0.5s) will always fail before the third one (t≈1.0s) succeeds. An optional micro-optimization would be an initial time.sleep(0.8) before entering the loop to skip two guaranteed misses. Whether this is worth the added complexity is a judgment call — the current approach is simpler and perfectly correct.


Potential concerns

None that would block merging. The change is minimal, correct, and well-motivated. The pytest.skip path (10s timeout) is a last resort for demo-side failures unrelated to the SDK itself.


Verdict

LGTM. Clean, minimal fix for a legitimate flaky test root cause. The polling strategy is sound, the fallback is appropriate, and the change touches nothing beyond what's necessary.

@TexasCoding TexasCoding merged commit 9592aa4 into main May 17, 2026
4 checks passed
@TexasCoding TexasCoding deleted the fix/integration-subaccount-cleanup branch May 17, 2026 14:11
TexasCoding added a commit that referenced this pull request May 17, 2026
…ers, order_groups) (#129)

* test(integration): poll for query-exchange visibility after POST

Demo's query-exchange replica lags writes by ~10 seconds. Orders and
order_groups created via POST aren't immediately readable via their
GET-by-id endpoints — the failing tests asserted GET success
immediately after CREATE and reliably hit 404 ("not_found" /
service: query-exchange).

Probe evidence (orders):
  create -> order_id X
  poll 1-3 (~1.5s): 404 not_found
  ...
  poll @ 10s: GET works, status=resting

Same pattern for order_groups (~9.5s).

- Added wait_for_resource (sync) and await_resource (async) helpers in
  tests/integration/helpers.py — bounded poll loop that swallows
  KalshiNotFoundError until timeout (default 15s, interval 0.5s) and
  re-raises if the resource never propagates.
- Wrapped GET-after-POST sites in test_orders.py
  (test_order_fill_lifecycle, test_create_get_cancel sync + async) and
  test_order_groups.py (test_create_and_get, test_update_limit,
  TestOrderGroupsAsync.test_create_get_delete).
- Dropped the now-redundant `await asyncio.sleep(0.5)` workaround +
  unused asyncio import from test_order_groups.py.

Verified locally: tests/integration/test_orders.py +
tests/integration/test_order_groups.py — 22/22 passed.

Closes 5 of 11 integration-test failures (#124).
Running tally: A (#126=4) + B (#127=1) + E (#128=1) + C (this=5) = 11/11.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review(#129): drop dead last_error var, use PEP 695 type params

Per bot review on PR #129:

- Removed unused last_error assignment in wait_for_resource (ruff F841;
  caused CI lint failure). bare 'raise' re-raises the active exc
  directly, so the captured ref was dead.
- Switched both helpers to PEP 695 type parameter syntax
  (def wait_for_resource[T](...)) to clear UP047. Drops the standalone
  TypeVar import.
- Expanded await_resource one-line docstring with the demo-lag context
  so the file reads top-to-bottom.

Verified: uv run ruff check . clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant