Skip to content

Control channel: scope buffer page reads to referenced pool entries and attachments#85

Closed
i-am-marvin wants to merge 6 commits into
mainfrom
claude/issue-80-20260714-2155
Closed

Control channel: scope buffer page reads to referenced pool entries and attachments#85
i-am-marvin wants to merge 6 commits into
mainfrom
claude/issue-80-20260714-2155

Conversation

@i-am-marvin

Copy link
Copy Markdown

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

Fixes #80

When a control-channel events page is served from the realtime sample buffer (open_sample_history_from_sample_history), the page limit only bounds the event rows. Each page request also unconditionally loads and json.loads the sample's entire message pool and call pool, and fetches every attachment body for the sample (tens of MB on image-heavy transcripts). Since the read runs synchronously on the eval's own event loop, a read-only monitor polling inspect ctl events imposes repeated blocking pauses on in-flight samples.

What is the new behavior?

Fix option 1 from the issue — page-scoped pool/attachment loading:

  • SampleHistory pools are now position-keyed mappings (dict[int, ...]) rather than dense lists. Full-history reads carry every entry (contiguous keys from 0); page reads carry only referenced positions.
  • After fetching a page's event rows, _sample_history(page_scoped=True) collects the input_refs/call_refs positional ranges and the attachment:// hashes referenced by the page's event JSON (plus refs inside the loaded pool entries, since large message content is itself stored as an attachment), and loads only those rows (WHERE pos IN (...) via ROW_NUMBER rank, WHERE hash IN (...) scoped to the sample; IN lists chunked under SQLite's bound-parameter limit).
  • Both paged entry points (open_sample_history_from and open_sample_history_tail) are page-scoped; open_sample_history (full reads: streaming flush, recorder, retry errors) still loads complete pools.
  • _expand_refs / resolve_model_event_inputs / resolve_model_event_calls / materialize_pooled_events accept position-keyed mappings alongside dense lists; the buffer transcript history provider resolves against the mappings.
  • SampleHistory.events_data (dense pools for .eval serialization) became a property that raises on a page-scoped (sparse) history rather than silently misaligning refs.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No — all changed interfaces are internal (inspect_ai.log._recorders.buffer.*, inspect_ai.event._pool). Public resolve_* helpers only widened their accepted parameter types.

Other information:

Verification: pytest tests/log tests/checkpoint tests/_control tests/_eval/test_retry_error_events.py (all passing), ruff check/ruff format, and mypy clean on changed files. New tests in tests/log/test_sample_history.py cover page-scoped message/call pool loading, attachment scoping (event-level and pool-entry-level), end-to-end materialization through BufferTranscriptHistoryProvider.events_from, and the events_data guard.

The keyset-cursor option (#2 in the issue) is not included; the ROW_NUMBER CTE still scans the sample's event rows per page, but no longer parses or ships unreferenced pool/attachment content.

🤖 Generated with Claude Code

Paged sample-history reads from the realtime buffer (open_sample_history_from
/ open_sample_history_tail) previously loaded and parsed the sample's entire
message pool, call pool, and every attachment body on every page request,
making control-channel event polls O(sample) instead of O(page).

SampleHistory pools are now position-keyed mappings; page-scoped reads
collect the input_refs/call_refs positions and attachment hashes actually
referenced by the page's events (including refs inside the loaded pool
entries) and fetch only those rows. Full-history reads still load complete
pools, and events_data (dense pools for .eval serialization) now guards
against misuse on page-scoped histories. Pool-ref resolution accepts
position-keyed mappings alongside dense lists.

Fixes #80

Co-authored-by: Ransom Richardson <1209015+ransomr@users.noreply.github.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@i-am-marvin i-am-marvin added the auto @auto: drive this PR autonomously (remove to stop) label Jul 14, 2026
@i-am-marvin

Copy link
Copy Markdown
Author

@review

Comment thread src/inspect_ai/log/_recorders/buffer/history.py
Comment thread src/inspect_ai/log/_recorders/buffer/history.py Outdated
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Reviewed the page-scoped pool/attachment loading. I verified the core invariants and found no correctness or security issues -- just two minor non-blocking nits, both around the events_data property (see inline comments on history.py).

What I checked:

  • ROW_NUMBER rank == pool position: pool rows are only ever deleted whole-sample (_remove_samples_now), and _get_pool_entries_at's ROW_NUMBER() OVER (ORDER BY id) - 1 assigns exactly the same positions as the old enumerate(_get_message_pool(...)) (which also ordered by id), so the invariant holds, including across resume/re-seed paths.
  • Constructor signature change is fully propagated: SampleHistory is constructed in only two places, both in database.py; the filestore raises NotImplementedError for all three history entry points.
  • events_data consumers (eval.py recorder, materialize_streaming_sample, eval_retry_error_from_history) all reach it via open_sample_history (full reads), so the page-scoped guard never fires on production paths.
  • Ref/attachment collection is complete: refs only ever live at input_refs and call.call_refs on top-level ModelEvents (pooling never applies to nested events), walk_model_call never aliases the call object itself to an attachment (only inner strings), and pool-entry-level attachment refs are walked. The zip realignment in __post_init__ is safe because TypeAdapter.validate_python raises rather than drops.
  • Parameter-limit chunking: 500 positions + 2 scope params stays under SQLite's 999 bound-parameter limit; all queries run inside the read transaction (generators are fully consumed before commit).

Verification: pytest tests/log tests/_control tests/_eval/test_retry_error_events.py (1156 passed), mypy and ruff check clean on the changed files.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

🔎 Review complete.

@i-am-marvin

i-am-marvin commented Jul 14, 2026

Copy link
Copy Markdown
Author

🤖 auto review rounds: 2 (cap 10).

Replace the contiguity heuristic in SampleHistory.events_data with an
explicit page_scoped field set by the construction site, so pages that
reference a contiguous pool prefix also raise instead of returning
truncated pools. Bind events_data to a local in
eval_retry_error_from_history to avoid rebuilding the dense pools twice.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@i-am-marvin

Copy link
Copy Markdown
Author

@review

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Re-reviewed at head 8632662 (round 2, after the previous review's nits were addressed). No correctness or security issues found, and no remaining nits.

Both prior suggestions are correctly resolved:

  • SampleHistory.events_data now keys off an explicit page_scoped flag rather than a key-contiguity heuristic, so a page referencing a contiguous pool prefix also raises — covered by test_page_scoped_history_rejects_events_data.
  • eval_retry_error_from_history binds history.events_data to a local, matching eval.py.

What I verified this round:

  • All events_data consumers use full histories. materialize_streaming_sample, eval_retry_error_from_history, and buffer_sample_streaming are all reached only via open_sample_history (full read, page_scoped=False), so the raising property cannot fire on existing paths; the paged entry points (open_sample_history_from/_tail) feed only the provider's materialization paths, which resolve refs via the position-keyed mappings.
  • ROW_NUMBER rank == pool position holds: pool tables are INTEGER PRIMARY KEY AUTOINCREMENT (ids monotone, never reused), rows are inserted per sample in pool-position order (_condense_model_event), and the full-read path derives positions from the identical ORDER BY id rank — so page-scoped and full reads cannot disagree.
  • Attachment scoping is complete for the "core" resolution the paged consumers use: hashes are collected from the page's event rows and from the loaded message/call pool entries (large message content lives as attachments inside pool entries), so nothing the materializer resolves can be missing. test_page_scoped_history_materializes_pool_entry_attachments covers the end-to-end path.
  • Transaction consistency: event rows and scoped pool/attachment reads all execute inside the same BEGINcommit, so a concurrent write cannot skew positions between the two queries.
  • Chunking stays under SQLite's bound-parameter limit (500 + 2 sample params per query), and the zip-realignment in __post_init__ is safe since TypeAdapter validation is 1:1 and dict key/value iteration order agree.
  • No nested-event blind spots: SubtaskEvent no longer carries child events, so top-level input_refs/call.call_refs extraction sees every ref a page can contain.
  • Ran pytest tests/log tests/checkpoint tests/_control tests/_eval/test_retry_error_events.py (1400 passed), plus mypy and ruff check on all changed files — all clean.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

🔎 Review complete — no outstanding suggestions.

@i-am-marvin

Copy link
Copy Markdown
Author

@ransomr 🤖 auto: the reviewer has no outstanding feedback — this PR has converged. auto never takes the final step (merge, or upstream promotion on the fork), so it's yours now; see the checks above for CI status. The auto label is left on, so auto will still react to new review or CI activity; remove it to disengage.

Add POOL_REF_FIELDS in event._pool as the single registry of event fields
carrying range-encoded pool refs, with collect_pool_ref_positions()
replacing the hardcoded collector in the buffer database. A new test
introspects the event models for *_refs fields and fails when one isn't
registered, so page-scoped reads can't silently drop pool entries for a
future ref field. Also add strict=True to the SampleHistory pool
re-keying zips so a length mismatch raises instead of truncating.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ransomr

ransomr commented Jul 16, 2026

Copy link
Copy Markdown
Member

@review full review please — latest push adds a shared POOL_REF_FIELDS registry (with an introspection test enforcing registration) and strict pool re-keying in SampleHistory.

…-80-20260714-2155

# Conflicts:
#	CHANGELOG.md
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review

Reviewed the full diff (page-scoped pool/attachment loading for buffer page reads). No correctness or security issues found - two non-blocking documentation/maintainability nits posted inline.

What I checked:

  • ROW_NUMBER position recovery (_get_pool_entries_at): matches the dense enumerate(... ORDER BY id) used by full reads exactly, so page-scoped and full reads share the same position-to-row assumption (including the INSERT OR IGNORE caveat). Chunked IN lists are disjoint (chunks of a sorted set), so no duplicate positions across chunks.
  • Attachment completeness for page reads: materialized events can only reference attachments appearing in the raw event JSON or in the loaded pool entries, and _sample_history collects hashes from both, so page-scoped resolution cannot miss a hash that full reads would have resolved.
  • events_data guard: all events_data consumers (eval.py recorder, streaming.py retry-error/flush paths, run.py) reach it only via full open_sample_history reads; the paged entry points are consumed exclusively through BufferTranscriptHistoryProvider, which now resolves against the sparse mappings. The explicit page_scoped flag (rather than key-shape inference) correctly handles the contiguous-prefix ambiguity, and a test covers it.
  • _expand_refs mapping branch: skipping absent positions mirrors slice truncation for out-of-range refs, so full-read behavior is unchanged.
  • __post_init__ re-keying: zip(..., strict=True) over insertion-ordered dicts is sound, and strict=True fails loudly rather than misaligning if a validator ever changed cardinality.
  • Registry test (test_pool_ref_registry_covers_all_ref_fields): good safety net - a new *_refs field fails CI until registered.
  • SQL construction: table names are Literal-typed, all values parameterized; chunking stays under the 999-variable limit.
  • SampleBufferFilestore does not implement the history entry points (raises NotImplementedError), so the interface change does not ripple there.
  • CHANGELOG entry is under ## Unreleased.

Verification run locally: pytest tests/log (707 passed), pytest tests/_control (411 passed), tests/_eval/test_retry_error_events.py (all passed), mypy and ruff check clean on changed files.

If you want the nits addressed, reply with a top-level @claude <instruction> comment (inline replies do not reach the dev agent).

Comment thread src/inspect_ai/event/_pool.py Outdated
Comment thread src/inspect_ai/log/_recorders/buffer/database.py
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

🔎 Review complete.

… histories on the SampleBuffer interface

Review round 2 feedback: move the export path's pool-ref remapping into
event._pool as remap_pool_refs(), walking the POOL_REF_FIELDS registry so
a newly registered ref field is remapped without a code change, and note
on the abstract open_sample_history_tail/_from docstrings that they yield
page-scoped histories (sparse pools, events_data raises).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@i-am-marvin

Copy link
Copy Markdown
Author

@review

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review

I reviewed the full diff (page-scoped pool/attachment loading for buffer page reads, the position-keyed SampleHistory pools, the POOL_REF_FIELDS registry, and the export-path remap refactor) and found no correctness bugs, security issues, or simplification opportunities to request. Notes on what I verified:

Position-recovery invariant — the ROW_NUMBER() OVER (ORDER BY id) - 1 rank in _get_pool_entries_at correctly recovers .eval pool positions: pool rows are inserted in index order within _condense_model_event (position derives from the in-memory index size, which gates add_message/add_call by hash so the INSERT OR IGNORE never actually suppresses a row), and buffer deletes are whole-sample only, so per-sample rank is stable for a live sample.

No dense-pool consumer sees a sparse history — all events_data consumers (.eval recorder, materialize_streaming_sample, eval_retry_error_from_history, run.py) reach it via open_sample_history full reads; the two paged entry points feed only mapping-aware resolution in transcript_history_provider, and the page_scoped guard on events_data correctly catches even the contiguous-prefix case the keys can't distinguish (nice test for that).

Attachment scoping — collection walks both the page's event JSON and the loaded pool entries, which covers large message content stored as attachments inside pooled messages; hash queries are sample-scoped and chunked under the bound-parameter limit (500 + 2 fixed params stays under 999).

Transaction consistency — all page-scoped queries execute eagerly inside the BEGINcommit window (the generators are consumed by the dict comprehensions in _sample_history), so pools/attachments are read from the same snapshot as the event rows.

Export remap refactorremap_pool_refs driven by POOL_REF_FIELDS preserves the old _remap_pool_refs semantics exactly, including copy-on-write of the nested call dict and leaving non-list refs untouched; the registry test's model walk genuinely covers nested *_refs fields.

Verification run locally: tests/log + tests/checkpoint (980 passed), tests/_control (433 passed), tests/log/test_sample_history.py + tests/_eval/test_retry_error_events.py (40 passed); mypy and ruff check/format --check clean on all changed files. CHANGELOG entry is under ## Unreleased; no submodule pointer changes.

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

🔎 Review complete — no outstanding suggestions.

@ransomr

ransomr commented Jul 16, 2026

Copy link
Copy Markdown
Member

@claude open the upstream PR

@i-am-marvin

i-am-marvin commented Jul 16, 2026

Copy link
Copy Markdown
Author

Claude finished @ransomr's task in 4m 8s —— View job


Opening the upstream PR

  • Sync branch with upstream main (merge, verify CHANGELOG placement and submodule pointer)
  • Push updated branch to fork
  • Open upstream PRblocked: no credential with upstream access (see below)
  • Close this fork PR — skipped, since the upstream PR isn't open yet

What was done

The branch is fully prepared for upstream promotion:

  • Fetched UKGovernmentBEIS/inspect_ai main (identical to fork main at 8117bf2ff) and merged it into this branch — clean merge, no conflicts (commit 005c5bbb3, pushed).
  • Verified the CHANGELOG entry still sits under ## Unreleased and the src/inspect_ai/_view/ts-mono submodule pointer is untouched.

Why I couldn't open the PR

I'm unable to create the upstream PR because both credentials available to this job are scoped to the meridianlabs-ai/inspect_ai fork only — the API call to POST /repos/UKGovernmentBEIS/inspect_ai/pulls returns 403 Resource not accessible by personal access token (and 403 Resource not accessible by integration for the Actions token). Opening it needs a human account (or a PAT) with access to the upstream repo. See the FAQ for token workarounds.

To open it yourself

Everything is staged — either use the compare link or the one command below:

Compare link: UKGovernmentBEIS/inspect_ai@main...meridianlabs-ai:inspect_ai:claude/issue-80-20260714-2155

Or via gh (org forks need explicit head_repo, which gh pr create lacks):

gh api repos/UKGovernmentBEIS/inspect_ai/pulls -X POST \
  -f \
  -f base="main" \
  -f head="claude/issue-80-20260714-2155" \
  -f head_repo="meridianlabs-ai/inspect_ai" \
  -F body=@body.md
Prepared PR body (save as body.md)
## This PR contains:
- [ ] New features
- [ ] Changes to dev-tools e.g. CI config / github tooling
- [ ] Docs
- [x] Bug fixes
- [ ] Code refactor

### What is the current behavior? (You can also link to an open issue here)

Fixes meridianlabs-ai/inspect_ai#80

When a control-channel events page is served from the realtime sample buffer (`open_sample_history_from``_sample_history`), the page `limit` only bounds the event rows. Each page request also unconditionally loads and `json.loads` the sample's **entire** message pool and call pool, and fetches **every** attachment body for the sample (tens of MB on image-heavy transcripts). Since the read runs synchronously on the eval's own event loop, a read-only monitor polling `inspect ctl events` imposes repeated blocking pauses on in-flight samples.

### What is the new behavior?

Page-scoped pool/attachment loading:

- `SampleHistory` pools are now **position-keyed mappings** (`dict[int, ...]`) rather than dense lists. Full-history reads carry every entry (contiguous keys from 0); page reads carry only referenced positions.
- After fetching a page's event rows, `_sample_history(page_scoped=True)` collects the `input_refs`/`call_refs` positional ranges and the `attachment://` hashes referenced by the page's event JSON (plus refs inside the loaded pool entries, since large message content is itself stored as an attachment), and loads only those rows (`WHERE pos IN (...)` via `ROW_NUMBER` rank, `WHERE hash IN (...)` scoped to the sample; `IN` lists chunked under SQLite's bound-parameter limit).
- Both paged entry points (`open_sample_history_from` and `open_sample_history_tail`) are page-scoped; `open_sample_history` (full reads: streaming flush, recorder, retry errors) still loads complete pools.
- `_expand_refs` / `resolve_model_event_inputs` / `resolve_model_event_calls` / `materialize_pooled_events` accept position-keyed mappings alongside dense lists; the buffer transcript history provider resolves against the mappings.
- `SampleHistory.events_data` (dense pools for `.eval` serialization) became a property that raises on a page-scoped (sparse) history rather than silently misaligning refs.
- A shared `POOL_REF_FIELDS` registry (`inspect_ai.event._pool`) is the single source of truth for which event-JSON fields carry pool refs; an introspection test fails CI if a new `*_refs` field is added without registering it, and export-path ref remapping is driven from the same registry.

### Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No — all changed interfaces are internal (`inspect_ai.log._recorders.buffer.*`, `inspect_ai.event._pool`). Public `resolve_*` helpers only widened their accepted parameter types.

### Other information:

Verification: `pytest tests/log tests/checkpoint tests/_control tests/_eval/test_retry_error_events.py` (all passing), `ruff check`/`ruff format`, and `mypy` clean on changed files. New tests in `tests/log/test_sample_history.py` cover page-scoped message/call pool loading, attachment scoping (event-level and pool-entry-level), end-to-end materialization through `BufferTranscriptHistoryProvider.events_from`, and the `events_data` guard.

A keyset-cursor alternative is not included; the `ROW_NUMBER` CTE still scans the sample's event rows per page, but no longer parses or ships unreferenced pool/attachment content.

Reviewed in meridianlabs-ai/inspect_ai#85.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Once the upstream PR is open, this fork PR should be closed with a comment linking to it (per CLAUDE.md).
· Branch

@ransomr

ransomr commented Jul 16, 2026

Copy link
Copy Markdown
Member

Superseded by the upstream PR: UKGovernmentBEIS#4523 (now the system of record).

@ransomr ransomr closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto @auto: drive this PR autonomously (remove to stop)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants