Skip to content

fix(knowledge): preserve concurrent dropped event batches - #3483

Open
HootRock wants to merge 3 commits into
ag2ai:mainfrom
HootRock:fix-concurrent-dropped-event-batches
Open

HootRock wants to merge 3 commits into
ag2ai:mainfrom
HootRock:fix-concurrent-dropped-event-batches

Conversation

@HootRock

Copy link
Copy Markdown

Why are these changes needed?

Two concurrent EventLogWriter.persist_dropped() calls can list the same existing segments and select the same next filename. Replacing that file with write() silently loses one complete batch, including when the writers share a SqliteKnowledgeStore.

Append each serialized batch using the existing atomic KnowledgeStore.append() contract and terminate it with a newline. Writers that select the same segment then retain both batches as valid JSONL. Numeric segment allocation and final snapshot replacement remain unchanged; concurrent batches may appear in either append order.

The regression forces both public list() calls to return their snapshots before either writer persists. It exercises real memory, disk, and SQLite backends, checks two multi-event batches, and verifies subsequent dropped and final events still load in order. All three cases fail on base cdc37c1845d9ef63c80263154b423ae3794d7b00 because one batch disappears, and pass with this change.

Local validation with Python 3.12.12 on Windows:

  • python -m pytest test/knowledge -q: 68 passed, 27 skipped (26 existing POSIX-only cases and the Redis module, whose dependency is absent).
  • python -m ruff check --no-fix ag2/knowledge/log.py test/knowledge/test_knowledge.py and python -m ruff format --check ag2/knowledge/log.py test/knowledge/test_knowledge.py: passed with Ruff 0.16.10.
  • git diff --check: passed.
  • Independent agent review found no actionable issues and separately ran all 13 event-log tests successfully.

The full repository suite, full pre-commit/mypy pipeline, Redis service, and cross-process filesystem behavior were not exercised locally. No model/provider calls were made.

Related issue number

Fixes #3482.

Checks

  • I've included any doc changes needed for https://docs.ag2.ai/. The affected method docstring now describes concurrent batches sharing a segment; no public API change.
  • I've added tests (if relevant) corresponding to the changes introduced in this PR.
  • I've made sure all auto checks have passed. Local checks above passed; upstream CI is pending.

AI assistance

Codex assisted with the diagnosis, code, regression test, PR text, and an independent agent review. The diff and claims were checked against the source and the observed failing/passing test results.

  • I understand the changes in this PR and can explain them in my own words.
  • I have verified that the PR description accurately reflects the actual diff.
  • If AI assistance was used, I reviewed, tested, and validated the generated code/text before submitting.

reviewed and tested locally.

@CLAassistant

CLAassistant commented Oct 25, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Oct 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
ag2/knowledge/log.py 93.84% <100.00%> (ø)

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
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.

[Bug]: Concurrent dropped-event writes overwrite a SQLite log segment

3 participants