db, api: Optimize database concurrency, query performance, and durability - #452
Draft
helq wants to merge 30 commits into
Draft
db, api: Optimize database concurrency, query performance, and durability#452helq wants to merge 30 commits into
helq wants to merge 30 commits into
Conversation
Add a submitted_at field to Event::RawMboxSubmitted, stamped with the server's current time when a raw mbox is submitted via POST /api/submit (Inject variant). This timestamp is propagated through metadata.received_date and used as the patchset date instead of the email's Date: header. This prevents stale mbox timestamps (which can be arbitrarily far in the past) from skewing queue ordering. The original email date is preserved in the messages table for display purposes. The Thread and Remote submission paths are unaffected as they already use server-side timestamps via create_fetching_patchset(). Signed-off-by: Elkin Cruz <elkin@google.com>
Define PriorityRule and CompiledPriorityRule structs for regex-based patchset priority classification. Add a custom serde deserializer (deserialize_indexed_vec) to handle both TOML array and env-var indexed-map representations. Add the priority_rules field to ReviewSettings with serde(default) so existing configs are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a priority INTEGER DEFAULT 500 column to the patchsets table and a composite index idx_patchsets_status_priority_date on (status, priority DESC, date ASC) for efficient priority-ordered queries. The column defaults to 500 so existing patchsets are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add migration to create the priority column and composite index. Introduce create_patchset_with_priority() which threads an explicit priority through all INSERT/UPDATE paths, with MIN(priority, ?) semantics to preserve manual deprioritization. Refactor create_patchset() to delegate with default None. Change get_pending_patchsets() ordering to priority DESC, date ASC. Add calculate_priority() for evaluating compiled regex rules against subjects (last match wins). Signed-off-by: Elkin Cruz <elkin@google.com>
Compile priority_rules from settings at startup and thread them through the DB worker into process_parsed_article(). Use calculate_priority() to compute priority from the patchset subject before calling create_patchset_with_priority(). API callers pass None for priority to create_fetching_patchset(). Signed-off-by: Elkin Cruz <elkin@google.com>
Expose the per-remote async mutex so that the fetcher module can acquire it before running git fetch, preventing concurrent fetches against the same remote (which causes duplicate multi-million-object downloads on large mirrors like the kernel repo). Signed-off-by: Elkin Cruz <elkin@google.com>
Introduce a fetch_queue table plus the DB operations that back the new durable fetch worker, modeled on the existing patchwork_outbox pattern. The table schema and FetchStatus/FetchQueueRow types are accompanied by the full set of queue operations: enqueue_fetch (idempotent), lock_pending_fetch (atomic claim under lease), mark_fetch_done, set_fetch_retry_at (backoff scheduling), mark_fetch_failed (terminal failure), sweep_ghost_fetches (reclaim expired leases), and get_stuck_fetch_placeholders (startup recovery for orphans). Also adds repo_url and priority columns to patchsets for crash recovery of in-flight fetches. This is the storage layer only; wiring lands in a follow-up. Signed-off-by: Elkin Cruz <elkin@google.com>
Add standalone utility functions that the upcoming FetchWorker will use, alongside the existing FetchAgent code: - PermanentFetchError: marker error for failures that should not be retried (e.g. missing commit with no remote to fetch from) - commit_hash_from_placeholder(): parse placeholder message ids (sha@sashiko.local or mr-N-base..head) into git commit/range - now_secs(): current unix timestamp helper - backoff_secs(): exponential backoff calculator (30s base, 10m cap) - looks_like_object_id(): detect full SHA vs ref name for efficient git fetch strategies Each function has unit tests. Signed-off-by: Elkin Cruz <elkin@google.com>
Rework git fetching around the fetch_queue table so requests survive restarts, cancellation, and worker crashes. FetchWorker polls the queue, claims due rows under a lease, ensures the commits are present (with an optimistic early-return when objects are already local, deliberately avoiding a full-remote fetch fallback that was pathologically slow on large mirrors), extracts patches, and emits PatchSubmitted events. Transient failures are retried with exponential backoff (30s doubling to a 10m cap); a fetch is only marked Failed after a continuous 24h retry window is exhausted. A periodic sweep reclaims ghost leases from dead workers. The per-remote lock (get_remote_lock) is acquired before every fetch to prevent concurrent fetches against the same remote, which previously caused duplicate multi-million-object downloads. The API submit paths now persist fetches via enqueue_fetch instead of pushing onto an mpsc channel, and the fetch_sender is removed from AppState/build_router/run_server. On startup, main backfills any patchset stuck in Fetching with no active queue row so in-flight work from before this change is recovered. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a nullable review_context column on patchsets that selects which review pipeline processes a patchset. NULL, the default, means the standard patch review; a JSON-encoded ReviewKind selects an alternate pipeline (currently cherry-pick review). The column is appended last so its NULL value is a free trailing field in SQLite, keeping patch-only deployments at zero extra storage. This commit defines only the storage shape and the enum; writing and reading the column arrive with their first users in later commits. Signed-off-by: Elkin Cruz <elkin@google.com>
Port the CherryPickContext type (renamed from the original hack's ConflictContext) and the finding filter into the new cherry_pick_review submodule. The filter drops low-severity and base-preexisting findings, drops original-patch-preexisting findings unless critical, and keeps resolution-introduced findings at medium or higher. The obsolete patch-body sentinel markers are dropped; the dispatch payload is persisted via the review_context column and ReviewKind instead. Includes filter and JSON round-trip unit tests. Signed-off-by: Elkin Cruz <elkin@google.com>
Add the cherry-pick-specific stage instructions, ported verbatim from the original hack so the pipeline stays behaviorally equivalent for benchmarking: - stage 1: semantic-intent analysis - stage 2: dropped-changes detection - stage 3: merge-correctness verification - origin classification (resolution_introduced, original_patch_preexisting, base_preexisting) The prompts are embedded via include_str so they ship with the binary regardless of the working directory. Shared stages 4-11 reuse the existing patch-review prompts and are not duplicated here. Signed-off-by: Elkin Cruz <elkin@google.com>
Express the merge-conflict resolution review (originally commit 66fb0a5) as a principled Pipeline over the shared stage machinery, instead of the inline hack in Worker::run. steps() runs cherry-specific analysis stages 1-3 (semantic intent, dropped changes, merge correctness) alongside the shared analysis stages 4-7, then the shared synthesis tail: dedup (8), concern resolution (9), verification (10), an origin-classification stage, a cherry-specific finding filter, and the final conflict report. The synthesis stages inject the accumulated concerns/findings into a per-stage prompt template. Those templates are ported verbatim from the original hack into synthesis.rs via the new StageSpec::with_template mechanism, so behaviour is preserved. The clean prompt equals the user prompt because it only affects logging, not the model call. build_context hydrates the three-commit context (subjects and the original patch diff) from git at review time, reusing the same static knowledge base and prefetched AST as the patch-review path. The database therefore only needs the minimal ReviewKind::CherryPick. Signed-off-by: Elkin Cruz <elkin@google.com>
Wire the alternate-pipeline selector end to end for the review binary. WorkerOptions gains a review_context field (Option<ReviewKind>), fed by a new --review-context JSON flag on the review CLI. When present in review_single_patch, the resolution commit is reviewed through the CherryPickReviewPipeline via execute_pipeline; when absent (the default), the existing Worker::run patch-review path is taken unchanged. The resolution SHA is the patch under review; the pipeline hydrates the rest of the three-commit context from git in build_context. This keeps the database footprint minimal: only the ReviewKind is threaded through. Signed-off-by: Elkin Cruz <elkin@google.com>
Add get_review_context / set_review_context to the Database, and have run_review_tool forward a patchset's stored review_context to the review subprocess via --review-context. Because run_review_tool already holds the database handle and patchset id, the selector is read there rather than threaded through every call site. Patchsets without a review_context (the default) are unaffected. Signed-off-by: Elkin Cruz <elkin@google.com>
Add a cherry-pick submit request that reviews an automated conflict resolution. It creates the usual fetching placeholder for the resolution commit, enqueues its fetch, and persists a minimal ReviewKind::CherryPick (original and optional base SHA) into review_context via set_review_context. The reviewer later forwards that selector to the pipeline, which hydrates the full three-commit context from git, so no bulky context is stored in the database. Signed-off-by: Elkin Cruz <elkin@google.com>
The config crate deserializes custom_remotes environment variables SASHIKO__GIT__CUSTOM_REMOTES__* as a map with numeric string keys instead of a sequence. This caused a deserialization failure when attempting to parse into a Vec structure. Replace the untagged enum deserialization strategy with a custom Visitor pattern in settings.rs. This Visitor successfully handles both sequential TOML/JSON arrays and indexed map formats, preserving type coercion for nested fields. Signed-off-by: Elkin Cruz <elkin@google.com>
When a custom remote has thousands of branches (e.g. icebreaker with 4,467 branches), fetching all refs downloads millions of objects and saturates the pod for hours. Add a branch_patterns field to CustomRemoteSettings that accepts glob patterns (e.g. "*/6.18", "14*"). When configured: 1. ensure_remote builds refspecs from patterns so git fetch only downloads matching refs instead of all branches 2. check_all_branches filters the branch list through the same patterns before adding baseline candidates Implement a simple glob matcher supporting "*" (any substring) and "?" (any single character) in git_ops, avoiding an external dependency for this small use case. Signed-off-by: Elkin Cruz <elkin@google.com>
When a patchset transitions to Pending, count how many sibling
patchsets share a similar timestamp (within a configurable window,
default 30s). If the count exceeds configured thresholds, cap
the priority of all patchsets in that window using MIN(priority, cap)
to avoid blocking the review queue with massive batch syncs.
Three new config options in [review]:
- batch_window_secs: time window for sibling detection (default 30)
- batch_tiers: list of {min_size, priority} threshold tiers
The deprioritization is applied retroactively to all patchsets in
the window on each new arrival, ensuring early arrivals in a batch
get deprioritized once the batch size becomes apparent. The MIN()
semantics ensure priority is never raised, preserving any manual
or rule-based deprioritization already in place.
Signed-off-by: Elkin Cruz <elkin@google.com>
Introduce ReplayProvider test infrastructure for deterministic replay of model interactions and tool responses. Add integration equivalence tests in tests/cherry_pick_pipeline_equivalence.rs verifying six core execution paths through CherryPickReviewPipeline. Signed-off-by: Elkin Cruz <elkin@google.com>
Add edge case test suite for cherry-pick review pipeline covering validation retries, truncated responses, recitation handling, and error fallbacks. Introduce a data-driven replay test harness that discovers test cases from tests/fixtures/replays/*, executes real tool operations on ToolBox against synthetic or configured Git repositories, and asserts against golden output and history logs. Include the sample_cache synthetic fixture demonstrating a full end-to-end multi-turn review of a buffer cache refactor. Signed-off-by: Elkin Cruz <elkin@google.com>
…into downstream-0.3.2-rebase
Add Default implementation and helper constructors DatabaseSettings::new and DatabaseSettings::memory to streamline test database setup. Add optional performance and durability configuration fields including mmap_size_mb, cache_size_kb, synchronous, and wal_flush_interval_secs with serde default attributes. Update all test cases to use the memory constructor helper. Signed-off-by: Elkin Cruz <elkin@google.com>
Configure SQLite performance pragmas on local database connections including synchronous = NORMAL, cache_size (default 64MB), mmap_size (default 256MB), and temp_store = MEMORY. These settings reduce fsync overhead during commits in WAL mode, expand in-memory page caching, and optimize sorting operations in RAM. Add unit test verifying pragmas are applied to Database instances. Signed-off-by: Elkin Cruz <elkin@google.com>
Implement Database::checkpoint_wal_passive and Database::spawn_wal_flusher to run periodic PASSIVE checkpoints on SQLite WAL. Spawn the flusher during daemon initialization in main.rs using the configured database.wal_flush_interval_secs interval (default 60s). This guarantees that dirty WAL pages are periodically checkpointed into the database file and flushed to persistent storage. Add unit test verifying WAL checkpoint metrics and flusher execution. Signed-off-by: Elkin Cruz <elkin@google.com>
Refactor get_patchsets query to use a Common Table Expression (CTE) to scope the findings aggregation subquery strictly to matching patchsets (r.patchset_id IN (SELECT id FROM p_lim)). Previously, get_patchsets performed a full-table join and aggregation across all 56,000+ reviews and findings on every paginated API/UI request. Add unit tests asserting pagination offsets and findings counts. Signed-off-by: Elkin Cruz <elkin@google.com>
helq
marked this pull request as draft
August 22, 2026 03:56
Add missing indexes on reviews.created_at and the composite findings(review_id, severity, preexisting) to accelerate review lookups, paginated review queries, and severity filtering. Update schema.sql and implement idempotent migration helper Database::migrate_perf_indexes_if_needed to ensure existing version 1 databases receive these performance indexes upon migration. Signed-off-by: Elkin Cruz <elkin@google.com>
Bound the maximum capacity of AsyncMapCache to prevent unbounded memory growth from parameterized subsystem queries. Expired entries are purged and LRU-like eviction is applied when full. Add a nocache boolean flag to SubsystemQuery to allow forcing fresh timeline statistics calculations for benchmarks and on-demand refreshes. Signed-off-by: Elkin Cruz <elkin@google.com>
Introduce a dedicated reader connection pool in Database to separate read-only query traffic from background worker write transactions in WAL mode. Local file databases initialize 4 reader connections with configured PRAGMAs and distribute read requests across them via round-robin. In-memory and remote databases safely fallback to sharing the primary connection. Signed-off-by: Elkin Cruz <elkin@google.com>
Under high-concurrency bot traffic, the default TCP listen backlog can cause new incoming TCP handshakes (such as GCP and Envoy load balancer health checks on /health) to stall in the socket backlog, leading to sporadic 503 no healthy upstream errors in browsers. Add a configurable tcp_backlog setting in ServerSettings (defaulting to 1024), configure socket2 with socket reuse and the configured backlog, and lower high-frequency patchset lookup logs from info to debug. Signed-off-by: Elkin Cruz <elkin@google.com>
helq
force-pushed
the
upstream/db-concurrency
branch
from
August 22, 2026 04:48
f0e1373 to
06d72a1
Compare
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.
On top of #450
This change resolves query stalls and improves API responsiveness under heavy review workloads by optimizing SQLite concurrency, indexing, and connection management.
Previously, high worker write volume and unindexed aggregations caused read lock contention and slow patchset listing queries.
Changes overview:
• Reader Pool: Added 4-connection reader pool to db.rs with lock-free round-robin borrowing for local SQLite databases.
• PRAGMA Tuning: Configured journal_mode = WAL, synchronous = NORMAL, cache_size = -64MB, mmap_size = 256MB, and busy_timeout = 5000ms.
• Periodic WAL Flusher: Background flusher task periodically triggers PRAGMA wal_checkpoint(PASSIVE) to control WAL file growth without blocking active readers.
• CTE Query Refactoring: Scoped findings severity aggregation in db.rs to only the paginated patchset IDs.
• Indexes & Migrations: Added idx_reviews_created_at and idx_findings_review_severity to schema.sql with dynamic idempotent startup migration.
• Cache Bounding: Bounded api.rs to 500 entries with TTL/timestamp eviction and added ?nocache=true parameter support.