database/rocksdb
1
0
Fork 0
mirror of https://github.com/facebook/rocksdb.git synced 2026-10-24 19:59:35 +03:00
Code Activity
13,865 Commits 290 Branches 299 Tags
fix_codex_review_command
Commit Graph

5 Commits

Author SHA1 Message Date
Peter Dillinger
52d8574629 Fix AI review workflows broken by cc8d9ea04 (#14696) 
Summary:
This commit fixes several potential issues introduced by cc8d9ea04 (CI: AI review workflow improvements) that broke the agent review jobs in GitHub CI.

1. Codex workflows fail when OPENAI_API_KEY is not configured:
   - Added Codex review workflows fail with exit code 1 if the API key secret is missing
   - Added script-level checks to skip Codex steps gracefully when OPENAI_API_KEY is unset, avoiding CI noise

2. Missing pull-requests: read permission in manual-review job:
   - The manual-review job calls github.rest.pulls.get() but lacked the required permission
   - Added 'pull-requests: read' to the manual-review job permissions

3. Potential crash when headSha is undefined:
   - parse-claude-review.js and parse-codex-review.js accessed meta.headSha.substring() without null check
   - Added defensive checks to handle undefined headSha gracefully

4. Claude model claude-opus-4-7 incompatible with thinking API:
   - The commit upgraded default model to claude-opus-4-7, but this model doesn't support the 'thinking.type.enabled' API used by the Claude Code action
   - Error: 'thinking.type.enabled' is not supported for this model. Use 'thinking.type.adaptive' and 'output_config.effort' to control thinking behavior
   - Reverted default model to claude-opus-4-6 and removed claude-opus-4-7 from options

Pull Request resolved: https://github.com/facebook/rocksdb/pull/14696

Test Plan: Created a draft PR with this change and draft production code changes, including a branch on facebook/rocksdb. Go to https://github.com/facebook/rocksdb/actions/workflows/claude-review.yml and choose "Run workflow" with that branch and PR. (Changes in the PR do are not picked up for the agent code review workflows; the only way to test what is outside of main is with a branch on facebook/rocksdb)

Reviewed By: joshkang97

Differential Revision: D103453631

Pulled By: pdillinger

fbshipit-source-id: 15a82257b5762e9e0b1b393dc45c4b343c866f7a
 2026-05-01 16:35:45 -07:00
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
 2026-04-30 09:37:58 -07:00
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
 2026-03-25 22:11:29 -07:00
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
 2026-03-24 15:39:27 -07:00
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
 2026-03-23 16:20:01 -07:00