Skip to content

πŸͺ² BUG-#28: Fix GenericProvider race between complete() and reload() - #44

Merged
FernandoCelmer merged 3 commits into
masterfrom
feature/28
Aug 15, 2026
Merged

πŸͺ² BUG-#28: Fix GenericProvider race between complete() and reload()#44
FernandoCelmer merged 3 commits into
masterfrom
feature/28

Conversation

@FernandoCelmer

@FernandoCelmer FernandoCelmer commented Aug 15, 2026

Copy link
Copy Markdown
Member

Description

Fixes a race condition in GenericProvider where a concurrent reload() could mutate url, model, headers, and other config fields mid-read inside complete(). Introduces a _ConnectionSnapshot frozen dataclass and a threading.Lock so complete() takes a single consistent snapshot before doing any I/O.

Motivation and Context

Closes #28. CodeLoop.ask() documents it is safe to call while run() is mid-turn on another thread, but GenericProvider had no protection against non-atomic config mutation during reload().

Types of changes

  • Bug fix

Checklist

  • Self-review done
  • Tests added
  • CHANGELOG updated
  • Docs updated

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _ConnectionSnapshot pattern is a clean solution for eliminating the torn-read problem. Two issues are worth addressing before merge.

Comment thread pycodeloop/providers/generic.py
Comment thread pycodeloop/providers/generic.py
@FernandoCelmer

Copy link
Copy Markdown
Member Author

[Blocking] _ConnectionSnapshot is missing request_builder

Problem β€” _ConnectionSnapshot is documented as "a consistent, point-in-time copy of everything reload() can mutate", but request_builder is absent from the dataclass. reload() reassigns self.request_builder inside the lock, and complete() currently calls self.request_builder(...) inside the same lock window β€” so for the call itself it is correct. But the snapshot abstraction is incomplete: any future helper that accepts a _ConnectionSnapshot and expects snap.request_builder will get an AttributeError, and any new code path that reads self.request_builder outside the lock will silently introduce a race.

Failure scenario β€” A developer adds def _build_body(self, snap: _ConnectionSnapshot, ...) and writes snap.request_builder(...). This compiles fine, fails at runtime with AttributeError: _ConnectionSnapshot has no field request_builder.

Fix β€” Add the field to the snapshot and populate it in _snapshot_locked():

@dataclass(frozen=True)
class _ConnectionSnapshot:
    ...
    request_builder: Callable  # add this

def _snapshot_locked(self) -> _ConnectionSnapshot:
    return _ConnectionSnapshot(
        ...
        request_builder=self.request_builder,
    )

Then replace self.request_builder(system_prompt, messages, tools, config.model) in complete() with config.request_builder(...).


[Suggestion] Lock is held during request body serialisation

Problem β€” complete() calls self.request_builder(system_prompt, messages, tools, config.model) inside with self._lock. Building the request body (serialising tool schemas and message arrays) is CPU-bound work. Holding the shared lock for this duration unnecessarily delays any concurrent reload() call.

Failure scenario β€” A caller passes a large tools list with complex nested schemas. reload() on another thread blocks for the full serialisation duration instead of just the microseconds needed to copy scalar fields.

Fix β€” Take the snapshot under the lock, release, then build the body:

def complete(self, system_prompt, messages, tools, ...):
    with self._lock:
        config = self._snapshot_locked()
    # lock released β€” body build is now contention-free
    body = config.request_builder(system_prompt, messages, tools, config.model)
    ...

@FernandoCelmer FernandoCelmer added the bug Something isn't working label Aug 15, 2026
… lock-free

_ConnectionSnapshot claimed to be a complete point-in-time copy of
everything reload() can mutate, but omitted request_builder β€” a
future reader of the snapshot expecting that field would hit an
AttributeError, and complete() was still reading self.request_builder
directly instead of the snapshot. Added the field and switched
complete() to read it off config; this also lets body construction
(schema serialization, potentially non-trivial) move outside the
lock now that the snapshot is fully self-contained, so it no longer
blocks a concurrent reload() for the duration of the build.
@FernandoCelmer
FernandoCelmer merged commit 8b04bc4 into master Aug 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CodeLoop.ask() is not safe to call concurrently with run() β€” shares provider and can interleave requests

1 participant