mirror of
https://github.com/facebook/rocksdb.git
synced 2026-11-08 03:52:33 -05:00
main
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Peter Dillinger
|
2a2e19f51b |
Remove Codex review support from GitHub CI (#15570)
Summary: Before this change, public GitHub CI supported both Claude and Codex AI code reviews through shared reusable workflows. Codex auto-review on PRs was already disabled (https://github.com/meta/rocksdb/issues/14933), but manual Codex reviews were still available via `/codex-review` and `/codex-query` PR comments. We don't expect to need Codex as an additional review layer, largely because internal reviews are more convenient, and Claude reviews are sufficient for public CI. This change removes the Codex wrapper workflows, the Codex comment parser script, and all Codex-specific steps from the shared review workflows. The shared workflows otherwise keep their provider-parameterized structure. If Codex review support is ever resurrected, it will need a security audit. Repo admins can delete the OPENAI_API_KEY secret after this lands. Pull Request resolved: https://github.com/meta/rocksdb/pull/15570 Test Plan: These workflows run from the base/default branch, so this PR's own CI does not exercise them; the first Claude review after merge will. Reviewed By: archang19 Differential Revision: D129398405 Pulled By: pdillinger fbshipit-source-id: 0628d20a61f6e9c5466d59443e422b71f00cfaf2 |
||
|
Peter Dillinger
|
0adcea5ea4 |
Update for GitHub repository move to meta/rocksdb (#15508)
Summary: The RocksDB GitHub repository moved from github.com/facebook/rocksdb to github.com/meta/rocksdb. Nearly all GitHub Actions jobs are gated on `github.repository_owner == 'facebook'` (to keep them from running on forks), so after the move those jobs were skipped on new PRs and pushes, and in the nightly and weekly schedules. This change updates the repository for the new location: * Fix: All `repository_owner` checks in the workflows now compare against 'meta', so the PR, nightly, weekly, and clang-tidy jobs run again. * Change: CI container images now come from ghcr.io/meta/rocksdb_ubuntu, and the Dockerfile build/push instructions match. * Change: Workflow names facebook/rocksdb/* are now meta/rocksdb/*. The Claude review workflow triggers on the pr-jobs workflow by name, so its `workflow_run` trigger is updated to match. (Required status checks are keyed by job name, not workflow name.) * Change: Every github.com/facebook/rocksdb URL in the repo now uses github.com/meta/rocksdb. This covers URLs used by CI and build scripts (third-party-deps release downloads, including DEPS_URL for Java builds; git remotes in setup-upstream and check_format_compatible.sh), the docs site config (`ghrepo`) and navigation, Maven SCM metadata, and links in code comments, docs, HISTORY.md, and blog posts. Historical links are included because GitHub's redirect from the old URL is not dependable for every link: some old issue URLs return 404 while the same issue under meta/rocksdb works. * Change: build_tools/format-diff.sh finds the upstream remote by matching either meta/rocksdb.git or facebook/rocksdb.git, because existing clones still use the old remote URL (which GitHub redirects for git operations). * Remove: The stale CircleCI status badge from README.md. CircleCI is no longer used. * Add: A final step in nightly's build-format-compatible job that fails if https://github.com/facebook/rocksdb stops redirecting to https://github.com/meta/rocksdb (or the old name is taken by another repo). Existing releases (e.g. DEPS_URL in java/Makefile as of v11.12), existing clones, and external links still depend on that redirect, so this alerts the RocksDB team if it breaks. The step runs even when the compatibility test fails. Not addressed here: runner labels (16-core-ubuntu, etc.) and secrets are configured per GitHub org, outside the repo. Any that are missing in the meta org need to be set up there. Pull Request resolved: https://github.com/meta/rocksdb/pull/15508 Test Plan: CI on this PR exercises the main fix: jobs should run instead of being skipped. Manual checks from an unrestricted network: * https://github.com/facebook/rocksdb returns a single 301 to https://github.com/meta/rocksdb, then 200, and `git ls-remote` via the old URL works. Old pull, wiki, blob, and release-download URLs redirect, but old issue URLs for https://github.com/meta/rocksdb/issues/9275, https://github.com/meta/rocksdb/issues/10080, and https://github.com/meta/rocksdb/issues/13293 returned 404 (https://github.com/meta/rocksdb/issues/12038 redirected) even though all four exist under meta/rocksdb. * A CI log showed ghcr.io/meta/rocksdb_ubuntu:22.1 pulling successfully. The new nightly step won't run before merge. The exact curl command it uses printed the expected `200 https://github.com/meta/rocksdb` when run manually. Separately, its script was extracted from the parsed workflow YAML and run against a stub curl: it passes for a redirect to meta, fails for a 404 and for a 200 without redirect, and reports a network failure distinctly. After merge, a manual workflow_dispatch of the nightly workflow can confirm it in CI. Reviewed By: archang19 Differential Revision: D128195050 Pulled By: pdillinger fbshipit-source-id: 3d42aa271b706eec2ef6c1586643056084fc866f |
||
|
Peter Dillinger
|
ec5106b3b8 |
Fix AI code review hiding details, stale+misleading driver (#15087)
Summary: The Claude/Codex CI reviewer was posting PR comments like "the review has been written to review-findings.md" instead of the review itself, and the comment footer cited claude_md/code_review.md as the review methodology even though CI stopped feeding that file to the model in https://github.com/facebook/rocksdb/issues/14507 (it switched to claude_md/ci_review_prompt.md). code_review.md had been orphaned from CI for months while still advertising itself as the CI methodology, so edits to it (e.g. the "post directly in your response" note) had no effect on CI. Separately, an over-long review exceeded GitHub's per-comment character limit and was rejected outright, losing the whole review. Changes: - ci_review_prompt.md: make the final response the review -- it becomes the PR comment verbatim -- and forbid ending the turn with a meta-summary such as "written to review-findings.md". review-findings.md is demoted to a recovery backstop; the per-phase incremental writes and the turn-limit recovery path are unchanged, so both the normal and recovery comment paths still produce a real review. - Repoint the "Review methodology" footer in parse-claude-review.js and parse-codex-review.js to claude_md/ci_review_prompt.md (the file CI actually uses), so the citation no longer points at a deleted file. - build-ai-review-comment.js: cap the assembled comment at GitHub's 65536-char limit, trimming only the model response (the sole unbounded input) and appending a pointer to the execution-log artifact, so an oversized review is truncated cleanly instead of rejected by GitHub. The header, footer, and <details> scaffolding are always preserved. - Delete claude_md/code_review.md (CI-dead since https://github.com/facebook/rocksdb/issues/14507; superseded locally). - Add claude_md/manual_code_review.md: a lightweight, single-reviewer local wrapper around ci_review_prompt.md (no multi-agent team/debate, no scratch files, inline output) for running a modest review by hand. It sources its methodology from ci_review_prompt.md so it cannot drift. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15087 Test Plan: - The workflow scripts have no CI unit coverage; verified both parse-*-review.js still parse, and that no reference to code_review.md remains anywhere. - Dry-ran manual_code_review.md by reviewing the current commit locally; it produced a right-sized single-reviewer report without spawning agents or writing files, and helped find+fix the comment limit issue. - Exercised the end-to-end GitHub CI path (per https://github.com/facebook/rocksdb/issues/14696, review-workflow changes not yet on main can only be tested from a branch on facebook/rocksdb): pushed these changes to the fix_hidden_review_details_test branch and ran the Claude Code Review workflow (claude-review.yml) via "Run workflow" against PR https://github.com/facebook/rocksdb/issues/15079, confirming the posted comment carries the full review inline (not a review-findings.md summary) with the ci_review_prompt.md footer. https://github.com/facebook/rocksdb/actions/runs/31537781229 https://github.com/facebook/rocksdb/pull/15079#issuecomment-5259127037 Reviewed By: xingbowang Differential Revision: D115635558 Pulled By: pdillinger fbshipit-source-id: d3ca7188162a086cd42d7b8bd8e60f3fc3df3bad |
||
|
Peter Dillinger
|
52d8574629 |
Fix AI review workflows broken by cc8d9ea04 (#14696)
Summary:
This commit fixes several potential issues introduced by
|
||
 xingbowang
|
cc8d9ea04d |
CI: AI review workflow improvements (#14659)
Summary: - Upgrade the Claude review workflow models and add complexity-based thinking budget selection for the main review run. - Keep AI review comments tied to the reviewed commit and restructure long reviews so high-severity findings stay visible while details live in a collapsible section. - Add early auto-trigger gating plus Codex review/comment workflows, including shared comment-building and parsing helpers. ## Testing - Not run. The branch refresh was a no-op rebase onto current `upstream/main`, and no new code changes were made during this task. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14659 Reviewed By: pdillinger Differential Revision: D102224743 Pulled By: xingbowang fbshipit-source-id: 39c25494062cbcd52d3e85f8a859b5a3907c758e |
||
 Xingbo Wang
|
0921ec1200 |
Improve Claude Code review: comprehensive prompt, incremental findings, recovery flow (#14507)
Summary: Improves the Claude Code review CI workflow to produce deeper, more reliable reviews. **Motivation:** The previous 30-turn limit caused reviews to silently produce "no output" on complex PRs (e.g. https://github.com/facebook/rocksdb/issues/14477). The review prompt was also too shallow — single-pass with no codebase context phase. **Changes:** **1. Comprehensive multi-agent review prompt** (`claude_md/ci_review_prompt.md`) - 9 specialized review agents: design, correctness, cross-component, invariant-adversary, caller-audit, performance, API, serialization, test coverage - Deep codebase context phase before agents spawn: caller-chain analysis (3-5 levels up), callee side-effect tracing, cross-component data consumer analysis, execution context verification, assumption stress-testing - Inter-agent debate with round-robin critique assignments - Final report quality rules: disproven findings removed, no stream-of-consciousness **2. Incremental findings + recovery flow** - `Write` tool added so Claude saves findings to `review-findings.md` after each phase - If the review hits the turn limit, a recovery step launches a Sonnet session to format partial findings into the standard output - Recovery file existence check prevents crash if recovery step fails - `getLastAssistantText` fallback truncated to 50KB to avoid enormous PR comments **3. Prompts extracted to files** (`claude_md/ci_*.md`) - `ci_review_prompt.md` — full review methodology - `ci_query_prompt.md` — `/claude-query` system prompt - `ci_recovery_prompt.md` — recovery formatting prompt - Secure: checkout is from base branch (main), not PR head **4. `max_turns` increased from 30 to 300** - Orchestrator budget for multi-agent workflow; sub-agents get their own turns - Recovery flow ensures partial results if limit is still hit Pull Request resolved: https://github.com/facebook/rocksdb/pull/14507 Reviewed By: archang19 Differential Revision: D98170111 Pulled By: xingbowang fbshipit-source-id: 390626a53e7a7f91c2d3e91ed4403494532425ed |
||
|
Xingbo Wang
|
ec31d0074f |
Accelerate build and test infrastructure (#14478)
Summary:
Reduce `make check` time from ~9.4 minutes to ~3.7 minutes (2.6x) on a 192-core machine, speed up CI by 1.8x across all jobs, and fix ccache hit rates (macOS cmake: 0.51% → 99.80%).
## Changes
### 1. Auto-detect lld linker (`build_detect_platform`)
- Probe for `lld` at configure time; fall back to default `ld.bfd` if unavailable
- Linux-only guard (`TARGET_OS = Linux`)
- **Measured: 12x faster linking** (7.4s → 0.6s per test binary)
- Add `-L/usr/local/lib` for lld library resolution on CI
- Install lld in CI pre-steps action
- Opt out with `ROCKSDB_NO_FAST_LINKER=1`
### 2. Sharded test execution (`Makefile`)
**Before:** `make check` enumerated every individual gtest case via `--gtest_list_tests`, then created a separate shell script for each case with `--gtest_filter=TestName`. For example, `block_based_table_reader_test` has 9,788 parameterized test cases — each spawned its own process, loading a ~50MB binary, initializing gtest, registering all ~10K tests, running exactly ONE, then tearing down. The per-process overhead was ~1.5s, so 9,788 × 1.5s ≈ 15,000s wasted on overhead alone. Across all 302 test binaries this produced 39,467 individual processes.
**After:** Use gtest's built-in sharding (`GTEST_TOTAL_SHARDS`/`GTEST_SHARD_INDEX`) to group ~10 test cases per process. Each shard loads the binary once and runs multiple tests sequentially — eliminating the per-process overhead. The 9,788 cases in `block_based_table_reader_test` become ~980 shards. Total process count drops from 39,467 to ~4,036. Same tests, same coverage, just fewer process spawns.
The shard count adapts to machine size: `min(ceil(test_count / GTEST_SHARD_SIZE), NCORES * 8)`. This ensures large machines (192 cores) get many small shards for parallelism, while small CI runners (4 cores) get fewer larger shards to avoid excessive overhead. Both `GTEST_SHARD_SIZE` (default 10) and `NCORES` can be overridden.
CI sharding uses round-robin distribution across 3 shards to balance heavy tests (db_test, db_compaction_test, etc.) instead of contiguous alphabetical ranges.
Note: gtest continues running all tests after a failure (`GTEST_THROW_ON_FAILURE=0`), so grouping tests does not mask failures — all failures within a shard are reported.
**Measured: 3.6x CPU reduction** (76,121s → 21,033s)
### 3. Fix SyncPoint leaks for test isolation (5 files)
Sharded execution exposed 43 test files that set SyncPoint callbacks but never clean them up. Stale callbacks with captured local variables cause segfaults or data corruption when subsequent tests in the same process trigger them.
**Systematic fix:** Global gtest `TestEventListener` in `testharness.cc` that calls `SyncPoint::DisableProcessing()` + `ClearAllCallBacks()` + `ClearTrace()` + `LoadDependency({})` after every test case. This cleans up all SyncPoint state: callbacks, dependency maps, cleared points, and the point filter. Registered via static initialization — no changes needed to individual test `main()` functions.
**SyncPoint infrastructure fixes:**
- `DisableProcessing()` now calls `cv_.notify_all()` to wake threads blocked in `Process()`. Previously, threads waiting for predecessor sync points that would never fire (because the test ended) would hang forever.
- `Process()` now rechecks `enabled_` after waking from `cv_.wait()`, so threads exit promptly when processing is disabled instead of looping forever.
**Specific fixes (defense-in-depth):**
- `SstFileReaderTest::VerifyNumEntriesCorruption` — leaked `PropertyBlockBuilder::AddTableProperty:Start` callback that corrupted SST entry counts
- `WritePreparedTransactionTest::CommitAndSnapshotDuringCompaction` — leaked `CompactionIterator:AfterInit` callback causing segfault via dangling pointers
- `TransactionTestBase` destructor — cleanup for 26 SyncPoint uses across transaction tests
- `RetriableLogTest` destructor — cleanup for log reader SyncPoint callbacks
### 4. Fix ccache for reliable cross-run caching (`setup-ccache`, `CMakeLists.txt`)
- **`CCACHE_COMPILERCHECK=content`**: Default `mtime` compared compiler binary modification time, which differs on every fresh CI runner → 0% hit rate. `content` hashes compiler output instead — stable across runner instances
- **`CCACHE_SLOPPINESS`**: CMake generates varying `-MF` paths and timestamps. Without sloppiness settings, ccache treated these as different compilations
- **`CMAKE_C/CXX_COMPILER_LAUNCHER`** instead of `RULE_LAUNCH_COMPILE`: Avoids double-wrapping ccache when also injected via PATH. Removes `RULE_LAUNCH_LINK` since ccache cannot cache link operations
- **macOS cmake hit rate: 0.51% → 99.80%**
### 5. Align GTEST_THROW_ON_FAILURE (`Makefile`)
- Set `GTEST_THROW_ON_FAILURE=0` in Makefile default to match CI's `pre-steps/action.yml`
- `=1` caused `std::terminate` in multi-threaded stress tests (e.g., `point_lock_manager_stress_test`, `rate_limiter_test`) when assertions fail — the gtest exception propagates across thread boundaries, triggering undefined behavior and heap-use-after-free under ASAN
### 6. Portability fixes
- `[[maybe_unused]]` instead of `__attribute__((unused))` for MSVC compatibility
- Remove blanket `-fPIC` from `COMMON_FLAGS` — only apply via `PLATFORM_SHARED_CFLAGS` for shared builds, preserving optimal codegen for release/static builds
- `make -s list_all_tests` to suppress make noise in test enumeration
## Results
### Local (192-core devvm, clean build)
| Metric | Before | After | Improvement |
|--------|--------|-------|-------------|
| Build time (clean) | 2m43s | ~2m | lld linker |
| Test wall clock | 400s | 200s | 2.0x |
| Test CPU total | 76,121s | 21,033s | 3.6x |
| Test failures | 0 | 0 | — |
| **Total make check** | **~9.4min** | **3m41s** | **2.6x** |
### CI (GitHub Actions)
| Metric | Before | After | Improvement |
|--------|--------|-------|-------------|
| Aggregate CI time | 15,220s | 8,425s | 1.8x |
| build-linux-arm | 763s | 221s | 3.5x |
| build-linux-release | 616s | 244s | 2.5x |
| build-windows-vs2022 | 2,868s | 1,205s | 2.4x |
| build-linux-java-static | 858s | 335s | 2.6x |
### ccache hit rates (all jobs)
| Job | Before | After |
|-----|--------|-------|
| macOS cmake (0-3) | 0.51% | **99.80%** |
| build-linux | 99.68% | **99.84%** |
| build-linux-clang18-asan-ubsan | 98.58% | **99.84%** |
| build-linux-clang18-mini-tsan | 98.74% | **99.84%** |
| build-windows-vs2022 | 99.22% | **99.83%** |
| All 27 ccache jobs | — | **>94%** |
## Remaining bottleneck
The critical path is now dominated by genuinely slow integration tests:
- `external_sst_file_test` shards: 97–113s each
- `db_test` shards: 109–124s each
- `db_bloom_filter_test`: 103s (non-parallel)
Further improvement would require test optimization or a `make check-fast` target.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14478
Test Plan:
- `make check -j192 SKIP_FORMAT_BUCK_CHECKS=1` — all 4,045 shards pass (ASAN+UBSAN), zero failures
- Verified SyncPoint fixes: each crash/corruption reproduces before fix, passes after
- Verified SyncPoint::DisableProcessing wakes blocked threads (previously caused indefinite hangs)
- Verified gtest does NOT stop after failure with `GTEST_THROW_ON_FAILURE=0` — all tests in a shard run and all failures are reported
- CI: all 34 checks pass across two consecutive runs
- ccache: validated hit rates >94% on second run after cache seeding
Reviewed By: hx235
Differential Revision: D97623305
Pulled By: xingbowang
fbshipit-source-id: b299e6d43c8713a9ef2b5659e8bbd74958afe155
|
||
|
Xingbo Wang
|
77a0e347bb |
Add Claude code review CI workflow (#14480)
Summary: Add GitHub Actions workflows for AI-powered code review on PRs using Claude (Anthropic). Follows the clang-tidy security pattern with two separate workflows for privilege separation. ## Trigger Modes **1. Auto** — Runs after `pr-jobs` workflow completes successfully via `workflow_run`. Safe for fork PRs (runs from default branch, never executes PR code). **2. Manual** — Maintainers comment `/claude-review [focus area]` or `/claude-query <question>` on any PR. Restricted to 15 authorized team members. **3. workflow_dispatch** — For manual testing. ## Security Model (Two-Workflow Separation) Same pattern as clang-tidy: **`claude-review.yml`** (analysis): - Runs Claude with `ANTHROPIC_API_KEY` - Has ONLY `contents: read` — no PR write, no issue write - Saves review markdown + metadata as artifact **`claude-review-comment.yml`** (posting): - Triggers on `workflow_run` completion - Downloads artifact and posts/updates PR comment - Has `pull-requests: write` but never runs AI This separation prevents a crafted PR from tricking Claude into exfiltrating write tokens. ## Review Methodology Review prompt in `claude_md/code_review.md` (shared with local Claude Code reviews). Five perspectives: - Call-chain analysis (3-5 levels up/down) - Correctness & edge cases - Cross-component & adversarial (10 execution contexts) - Performance - API compatibility & test coverage ## Shared Scripts - `.github/scripts/post-pr-comment.js` — Create-or-update PR comment with marker-based dedup. Now used by both clang-tidy and Claude review. - `.github/scripts/parse-claude-review.js` — Parses `claude-code-base-action` execution log into markdown. ## Files Changed | File | Description | |------|-------------| | `.github/workflows/claude-review.yml` | Analysis workflow (476 lines) | | `.github/workflows/claude-review-comment.yml` | Comment posting workflow (146 lines) | | `.github/scripts/post-pr-comment.js` | Shared PR comment utility (57 lines) | | `.github/scripts/parse-claude-review.js` | Execution log parser (78 lines) | | `.github/workflows/clang-tidy-comment.yml` | Updated to use shared script | | `claude_md/code_review.md` | Review methodology (104 lines) | ## Setup Required Add `ANTHROPIC_API_KEY` secret to the repo settings. ## Testing Tested end-to-end on `xingbowang/rocksdb` fork — both auto and manual triggers, artifact upload/download, comment posting, and duplicate detection all verified working. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14480 Reviewed By: omkarhgawde Differential Revision: D97832666 Pulled By: xingbowang fbshipit-source-id: f80c7d8683ac980614dc4ca66c1e545deb3be504 |