Concurrency hardening for the async SDK - #60
Merged
Conversation
Adds deterministic concurrency tests for the async client and its generated sync twin. Interleaving is driven by Event/Barrier handshakes rather than wall-clock sleeps, so the ordering under test is fixed. Tests assert the invariant (a failure surfaces as CodesphereError), not the exception class, so they encode what callers depend on and survive the fixes that follow. Eleven of these fail against the current client, each for a distinct defect: unrefcounted close(), a captured client across retry backoff, cross-thread TOCTOU in the sync open(), a lost flags invalidation, a retried DELETE whose 404 masks success, and in-place model mutation that diverges from server write ordering.
Sharing one SDK across tasks was unsafe: open() was idempotent but close() was not, so the first scope to exit tore the transport down for everyone else. open()/close() are now reference counted behind _compat.Lock, which also closes the check-then-act window that let two threads in the sync twin each build a transport and orphan one. The teardown awaits __aexit__ outside the lock so a slow drain cannot block a concurrent open() of the next transport. request() now re-resolves the client on every retry attempt instead of capturing it once, so a close during backoff is caught by the SDK rather than surfacing as httpx's bare RuntimeError. That error is remapped to the new ClientStateError(CodesphereError, RuntimeError) only when the client is actually gone; unrelated httpx internals still propagate. The dual base keeps existing `except RuntimeError` handlers working. test_open_is_idempotent asserted the old (buggy) teardown semantics and is replaced by tests for reuse, balanced counting, and close underflow.
invalidate() cleared the snapshot outside the lock, so a get() already awaiting its request would overwrite the None with data fetched before the invalidation. The invalidation evaporated and the next read served the stale snapshot. A fetch now stamps the generation it started under and installs its result only if invalidate() has not bumped it meanwhile. invalidate() stays synchronous: taking the lock there would block the caller for the duration of an in-flight flags request. require() also read _legacy_platform outside the lock after get() returned, so a concurrent refresh could pair a snapshot with the legacy flag of a different fetch. _fetch() now returns both, and the new _get_with_legacy() hands them to callers from one lock acquisition.
When a DELETE reached the platform and only its response was lost (a gateway 503), the retry saw 404 and the SDK raised NotFoundError for an operation that had in fact succeeded. Tearing down a landscape reported "no landscape deployed" precisely because the teardown worked. A 404 is now treated as success when the request is a DELETE and at least one retry has already run. The attempt > 0 guard matters: a 404 on the first attempt is a genuine miss and still raises. The rule is scoped to DELETE, so a retried PUT that 404s is unaffected. Safe for every current DELETE operation: all five declare response_model=NoneType, so the response body is never parsed. The RetryConfig docstring now states the residual risk this does not solve. PUT and DELETE are idempotent as methods but not always in effect, and without idempotency keys the SDK cannot stop a retried teardown from executing twice. Callers who cannot tolerate that are pointed at max_retries=0.
BREAKING CHANGE: reading a field on a model after update() raises StaleModelError until refresh() is called. Writes copied values back into the local model, which is wrong by construction under concurrency: the platform orders writes by arrival while the client sees them by response. Two tasks updating the same workspace could leave the model reporting "first" while the server held "second", permanently and silently. Workspace.update() wrote back the request payload; the Domain methods wrote back the server's response, which is authoritative for the moment it was produced but can still be overtaken by a concurrent write. Both are now handled the same way: the instance is marked stale. Staleness drops the field values from __dict__ so reads route through __getattr__, which costs nothing until a model actually goes stale. Identity fields survive, so a stale instance can still be logged and re-fetched, and repr() marks it. model_dump/model_dump_json are guarded too: pydantic serializes straight from __dict__, so without that a stale model would quietly dump only its identity. Workspace.refresh() and Domain.refresh() re-read the entity in place. Domain writes still return the server's response; prefer it over self. Removes codesphere.utils.update_model_fields, now unused.
wait_for_stage() could report success for a pipeline run the caller never started: if someone else redeployed mid-wait, it simply observed whatever run was current. The API exposes no run id, but started_at changes on restart, so the run is pinned on first sighting and a different value now raises ConflictError. Both wait_for_stage() and wait_until_running() counted elapsed time by summing poll_interval, ignoring how long the status requests took. A slow endpoint could overrun the timeout by a wide margin (5.15s against a 0.2s budget in the regression test). Both now measure against a monotonic deadline and never sleep past it. LogStream is backed by one SSE response body that can only be read once. Re-entering it stranded the first stream context, and a second iterator surfaced a raw httpx.StreamConsumed. Both now raise ClientStateError explaining that streams are single-use. Adds docs/guides/concurrency.md. Alongside the guarantees, it states plainly what the SDK cannot do: with no ETags, If-Match, or idempotency keys, read-modify-write cycles have an unclosable lost-update window and retried writes can execute twice.
unasyncd exits non-zero whenever it transforms a file, the way a formatter signals "changed". Since run() defaulted to fatal, the very run that did the work aborted before restore_all_blocks() and before ruff normalized the generated tree — so `make sync-gen` failed on every real change and had to be run twice to produce a correct result.
🛡️ Bandit Security Scan Results✅ No security issues found by Bandit. |
Test Execution Summary
|
Integration Test Results65 tests 0 ✅ 14s ⏱️ For more details on these failures and errors, see this check. Results for commit 424b98e. |
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.
An audit of
src/codesphere/_asyncfound six concurrency defects. Each wasreproduced with an executable probe before being fixed, and each has a
regression test that fails against the previous code.
The async client is documented as the choice for code that "makes many
concurrent API calls", but the repo had essentially no concurrency coverage —
one
asyncio.gatherin the entire suite.Verified fixed
Re-run of the original audit probes against this branch:
async withscopesB: RuntimeError (client closed)B: okbuiltins.RuntimeErrorClientStateErroropen()cross-thread TOCTOUconstructed=2, orphaned=1constructed=1, orphaned=0invalidate()vs in-flight fetchcached: True (fetches=1)cached: False (fetches=2)DELETE404caller sees -> NotFoundErrorcaller sees -> oklocal reports 'A' (DIVERGED)local read -> StaleModelErrorWhat changed
open()/close()are reference counted behind_compat.Lock,so nested and concurrent scopes no longer tear the transport down for each
other, and two threads racing
open()in the sync twin can no longer orphan aconnection pool.
request()re-resolves the client per retry attempt; httpx'sbare
RuntimeErrormaps to the newClientStateError(CodesphereError, RuntimeError).invalidate()win against anin-flight fetch while staying synchronous.
_legacy_platformand the snapshotnow come from one lock acquisition.
DELETEcounts as success. Guarded onattempt > 0, scoped toDELETE. Per the platform's lack of idempotency keys,retries otherwise stay as-is and
RetryConfigdocuments that a retried writecan execute twice.
Workspace.update()and the three
Domainwrite methods mark the instance stale; reads andto_dict()/to_json()/to_yaml()raiseStaleModelErroruntilrefresh().Identity fields stay readable.
wait_for_stage()pins its run bystarted_atandraises
ConflictErrorif someone else restarts the stage. Both polling loopsmeasure
timeoutagainst a monotonic deadline.LogStreamis guarded assingle-use.
what the SDK cannot do: with no ETags,
If-Match, or idempotency keys,read-modify-write has an unclosable lost-update window.
Breaking change
Reading a field after
update()now raisesStaleModelErrorinstead ofreturning a value that may contradict the platform.
CHANGELOG.mdcovers themigration; needs a minor version bump before release.
Also removes
codesphere.utils.update_model_fields(no remaining callers) andfixes
scripts/gen_sync.py, which aborted before post-processing on every runthat actually transformed a file.
Verification
505 tests pass;
ruff,ty,make sync-check, and a strict docs build are allclean.
🤖 Generated with Claude Code