perf(audit): use WAL journal to avoid fsync per commit on event loop - #392
Open
hi-neason wants to merge 1 commit into
Open
perf(audit): use WAL journal to avoid fsync per commit on event loop#392hi-neason wants to merge 1 commit into
hi-neason wants to merge 1 commit into
Conversation
AuditStore.append runs synchronously from TurnEngine._audit at every tool stage (17 call sites inside the async engine loop). With SQLite's default DELETE journal + synchronous=FULL, each append's commit() fsyncs, blocking the event loop for ~1-10ms per tool call. Set journal_mode=WAL and synchronous=NORMAL at connect time so a commit returns without an fsync while preserving transaction atomicity; WAL checkpoints flush to disk periodically. Add tests/test_audit.py covering the pragma, round-trip/filters, secret/body redaction, browser_type input redaction, resource extraction, close idempotency, and truncation.
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
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.
Summary
While reading through the engine I kept seeing
self._audit(...)called directly inside the async loop — 17 of them — and each one ends up atAuditStore.append, which does anINSERT ... commit()against SQLite.AuditStorenever sets a journal mode, so SQLite runs with the defaults:DELETEjournal +synchronous=FULL. That means every audit commit is an fsync. With a couple of tool calls per turn that's a handful of syscalls, but on a busy turn (multiple tools, retries, plan/approval stages) it adds up, and because the sink is invoked synchronously from insideTurnEngine._audit, each fsync parks the whole event loop — roughly 1–10ms per audited tool stage on a typical disk.The fix is two lines at connect time:
PRAGMA journal_mode=WALPRAGMA synchronous=NORMALWAL means writers don't block readers and commit doesn't fsync;
synchronous=NORMALis the recommended pairing with WAL and is still durable across crashes (the WAL checkpoints to the main DB on close/rotation). Transaction atomicity is unchanged — a crash mid-commit still rolls back. No API or behavior change beyond the pragma.Change
coworker/audit.py— set both pragmas right after connecting, before theCREATE TABLE IF NOT EXISTSruns. Added a short comment explaining why (since the reason for the pragma is non-obvious without knowing how hot_auditis).Tests
I noticed
coworker/audit.pyhad no test file at all, even though it's responsible for redacting secrets before they're written tocoworker.db. So along with the regression test for the pragma I addedtests/test_audit.pycovering the parts that are silent if they regress:test_audit_store_uses_wal_journal— assertsjournal_mode == "wal"andsynchronous == 1(NORMAL). This is the one that actually catches the perf bug; onmainit fails withassert 'delete' == 'wal'.test_append_and_list_roundtrip,test_list_filters— basic storage shape and thesession_id/toolfilters.test_sanitize_args_redacts_secrets— every key in_SECRET_KEYS/_BODY_KEYSplus the*_tokensuffix path.test_sanitize_args_redacts_browser_type_text—browser_type'stextfield gets[redacted input](form input, not a body blob).test_resource_extracts_well_known_keys— url / owner-repo / zendesk subdomain / result-URL fallback.test_close_is_idempotentandtest_summarize_truncates_long_strings.I confirmed the new pragma test fails without the fix (stashed
audit.py, sawassert 'delete' == 'wal', then popped) and passes with it.Full suite: 1006 → 1014 passed, 1 skipped. The 9 failures present locally (
test_fake_slack×8 needs themessagingextra /slack-bolt, plus onetest_ui_refresh_e2e) are pre-existing on a cleanmainand unrelated to this change.No screenshots — backend-only (SQLite pragma + tests). Happy to add anything else that'd help review.
Checklist
pytest tests -qpassesruff check/ruff format --checkclean on the added test file