Bound full smoke Git workspace workload#525
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughShorten the local-e2e smoke-all step timeout and add Git workspace env/timeouts; README clarifies smoke coverage is bounded to ChangesGit workspace smoke test timeout and pinning + runner restore change
Sequence Diagram(s)sequenceDiagram
participant Actions as GitHub_Actions
participant Runner as feature-matrix-runner.sh
participant Drive9 as drive9_cli
participant Workspace as Git_Workspace
Actions->>Runner: trigger "Run full smoke-all suite" (env includes GIT_WORKSPACE_*, MOUNT_READY_TIMEOUT_S)
Runner->>Drive9: start_mount (--foreground, clone with GIT_WORKSPACE_CLONE_TIMEOUT_S)
Drive9->>Workspace: mount filesystem
Runner->>Drive9: stop_mount (--pack restore_pack_archive --pack-path .git)
Runner->>Drive9: start_mount (--unpack restore_pack_archive --foreground)
Runner->>Actions: publish step conclusions for summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54911ead97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RUN_GIT_WORKSPACE_SMOKE: "1" | ||
| GIT_WORKSPACE_CLONE_TIMEOUT_S: "1800" | ||
| GIT_WORKSPACE_REPOS: "drive9=https://github.com/mem9-ai/drive9.git" | ||
| GIT_WORKSPACE_SCENARIOS: "agent_edit_add_commit" |
There was a problem hiding this comment.
When scheduled local-e2e or a manual run_e2e_smoke_all=1 run reaches this step, overriding GIT_WORKSPACE_SCENARIOS to only agent_edit_add_commit drops the fast_worktree scenario that is the only e2e path exercising drive9 git worktree add --fast/remove --fast. I checked the separate Git feature matrix and it covers clone/diff/restore flows but not worktree add/remove, so this change leaves that production CLI flow without the daily hard-fail coverage the smoke script previously provided.
Useful? React with 👍 / 👎.
|
Updated head Findings from
Fixes in
Validation:
New all-flags run: https://github.com/mem9-ai/drive9/actions/runs/27150267937. |
|
Follow-up after checking all-flags run 27150267937. Findings:
Fix in head
Validation before push:
Fresh runs:
Expected final state: smoke-all and Git feature matrix should be clean; if the manual gate remains red only because |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e/git-workspace-smoke-test.sh (1)
269-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTear down failed foreground mounts before returning.
With
--foreground, the process started at Line 274 stays attached until unmount. If Line 276 or Line 277 times out,start_mountreturns nonzero but leaves that mount process running, so a late mount can interfere with later checks or the next scenario. Mirror the cleanup path already used ine2e/_feature-matrix-runner.sh.🛠️ Proposed fix
args+=( :/ "$MOUNT_POINT" ) drive9 "${args[@]}" >>"$MOUNT_LOG" 2>&1 & MOUNT_PID="$!" - wait_mount_state mounted - wait_mount_log_ready "$MOUNT_LOG" + if wait_mount_state mounted && wait_mount_log_ready "$MOUNT_LOG"; then + return 0 + fi + if [ -n "${MOUNT_PID:-}" ] && kill -0 "$MOUNT_PID" >/dev/null 2>&1; then + kill "$MOUNT_PID" >/dev/null 2>&1 || true + wait "$MOUNT_PID" >/dev/null 2>&1 || true + fi + tail -n 80 "$MOUNT_LOG" >&2 || true + return 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/git-workspace-smoke-test.sh` around lines 269 - 277, The foreground mount started with drive9 (args, MOUNT_PID) can be left running on failure; update the start sequence around drive9, MOUNT_PID, wait_mount_state and wait_mount_log_ready so that if wait_mount_state or wait_mount_log_ready times out/fails you kill/terminate the foreground mount process (use MOUNT_PID), wait for it to exit, and remove/cleanup the mount state before returning non-zero; mirror the teardown logic used in e2e/_feature-matrix-runner.sh to ensure the background foreground process is properly stopped on any error path.e2e/_feature-matrix-runner.sh (1)
1174-1180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the checkpoint after writing the non-durable sentinel.
restore_pack_archiveis produced at Line 1115, butbuild/cache.tmpis only created at Lines 1174-1175 and the remount at Line 1178 still unpacks that older archive. This check stays green even ifbuild/**starts being checkpointed, because the file was never part of the artifact under test. Move the sentinel creation before Line 1115, or regenerate the archive after writing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/_feature-matrix-runner.sh` around lines 1174 - 1180, The test writes the non-durable sentinel "build/cache.tmp" after the checkpoint archive (restore_pack_archive) is produced, so the remount unpacks an archive that never contained the sentinel; update the script so the sentinel file is created before producing restore_pack_archive or, alternatively, recreate/regenerate restore_pack_archive immediately after writing "$restore_repo/build/cache.tmp" so the later start_mount/unpack (start_mount with "$restore_pack_archive") uses an archive that actually reflects the sentinel; locate the archive creation at the restore_pack_archive generation step and modify that flow or add a regeneration step before the start_mount/--unpack call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@e2e/_feature-matrix-runner.sh`:
- Around line 1174-1180: The test writes the non-durable sentinel
"build/cache.tmp" after the checkpoint archive (restore_pack_archive) is
produced, so the remount unpacks an archive that never contained the sentinel;
update the script so the sentinel file is created before producing
restore_pack_archive or, alternatively, recreate/regenerate restore_pack_archive
immediately after writing "$restore_repo/build/cache.tmp" so the later
start_mount/unpack (start_mount with "$restore_pack_archive") uses an archive
that actually reflects the sentinel; locate the archive creation at the
restore_pack_archive generation step and modify that flow or add a regeneration
step before the start_mount/--unpack call.
In `@e2e/git-workspace-smoke-test.sh`:
- Around line 269-277: The foreground mount started with drive9 (args,
MOUNT_PID) can be left running on failure; update the start sequence around
drive9, MOUNT_PID, wait_mount_state and wait_mount_log_ready so that if
wait_mount_state or wait_mount_log_ready times out/fails you kill/terminate the
foreground mount process (use MOUNT_PID), wait for it to exit, and
remove/cleanup the mount state before returning non-zero; mirror the teardown
logic used in e2e/_feature-matrix-runner.sh to ensure the background foreground
process is properly stopped on any error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7db5107-e52e-4e1f-a7ce-336197179a9c
📒 Files selected for processing (2)
e2e/_feature-matrix-runner.she2e/git-workspace-smoke-test.sh
|
Follow-up for all-flags gate
Validation: workflow YAML parse, expression audit, and Fresh runs:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens and stabilizes the local E2E workflow by bounding the “smoke-all” runtime, narrowing Git workspace smoke coverage to a single repo/scenario, and improving mount lifecycle behavior/restore durability in the Git feature matrix runner.
Changes:
- Add a 35-minute timeout to the “full smoke-all” workflow step and bound Git workspace smoke to
mem9-ai/drive9+agent_edit_add_commit. - Run Drive9 mounts in
--foregroundmode in the Git workspace smoke test and feature-matrix runner (enabling better process tracking/cleanup semantics). - Extend the Git feature-matrix restore suite to pack/unpack
.gitstate across unmount/remount and propagate failures with non-zero returns.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
e2e/README.md |
Documents the newly bounded Git workspace smoke coverage within smoke-all, keeping the broader Git matrix as a separate hard-fail step. |
e2e/git-workspace-smoke-test.sh |
Runs drive9 mount with --foreground to make mount process behavior more deterministic under CI. |
e2e/_feature-matrix-runner.sh |
Runs mounts with --foreground, adds pack/unpack of .git on restore flows, and returns non-zero on key mount failures. |
.github/workflows/local-e2e.yml |
Adds step timeout and new env bounds; updates summary/enforcement logic (but currently introduces an enforcement regression via .conclusion). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "| Check | Outcome |" | ||
| echo "| --- | --- |" | ||
| echo "| API smoke | ${{ steps.api-smoke.outcome || 'skipped' }} |" | ||
| echo "| Existing-key smoke | ${{ steps.existing-key-smoke.outcome || 'skipped' }} |" | ||
| echo "| CLI smoke | ${{ steps.cli-smoke.outcome || 'skipped' }} |" | ||
| echo "| FUSE release gate | ${{ steps.fuse-smoke.outcome || 'skipped' }} |" | ||
| echo "| FUSE performance compare | ${{ steps.fuse_performance_compare.outcome || 'skipped' }} |" | ||
| echo "| FUSE concurrency stress | ${{ steps.fuse-concurrency-stress.outcome || 'skipped' }} |" | ||
| echo "| FUSE POSIX/fsx gate | ${{ steps.fuse-posix-fsx.outcome || 'skipped' }} |" | ||
| echo "| Full smoke-all suite | ${{ steps.smoke-all.outcome || 'skipped' }} |" | ||
| echo "| Git feature matrix | ${{ steps.git-feature-matrix.outcome || 'skipped' }} |" | ||
| echo "| API smoke | ${{ steps.api-smoke.conclusion || 'skipped' }} |" | ||
| echo "| Existing-key smoke | ${{ steps.existing-key-smoke.conclusion || 'skipped' }} |" | ||
| echo "| CLI smoke | ${{ steps.cli-smoke.conclusion || 'skipped' }} |" | ||
| echo "| FUSE release gate | ${{ steps.fuse-smoke.conclusion || 'skipped' }} |" | ||
| echo "| FUSE performance compare | ${{ steps.fuse_performance_compare.conclusion || 'skipped' }} |" | ||
| echo "| FUSE concurrency stress | ${{ steps.fuse-concurrency-stress.conclusion || 'skipped' }} |" | ||
| echo "| FUSE POSIX/fsx gate | ${{ steps.fuse-posix-fsx.conclusion || 'skipped' }} |" | ||
| echo "| Full smoke-all suite | ${{ steps.smoke-all.conclusion || 'skipped' }} |" | ||
| echo "| Git feature matrix | ${{ steps.git-feature-matrix.conclusion || 'skipped' }} |" |
| check_required() { | ||
| local name="$1" | ||
| local outcome="$2" | ||
| if [ "$outcome" != "success" ]; then | ||
| echo "$name did not succeed (outcome: $outcome)" | ||
| local conclusion="$2" | ||
| if [ "$conclusion" != "success" ]; then | ||
| echo "$name did not succeed (conclusion: $conclusion)" |
| check_required "API smoke" "${{ steps.api-smoke.conclusion }}" | ||
| check_required "Existing-key smoke" "${{ steps.existing-key-smoke.conclusion }}" | ||
| check_required "CLI smoke" "${{ steps.cli-smoke.conclusion }}" | ||
| check_required "FUSE release gate" "${{ steps.fuse-smoke.conclusion }}" | ||
| if [ "${COMPARE_FUSE_PERFORMANCE_METRICS}" = "1" ]; then | ||
| check_required "FUSE performance compare" "${{ steps.fuse_performance_compare.outcome }}" | ||
| check_required "FUSE performance compare" "${{ steps.fuse_performance_compare.conclusion }}" | ||
| fi | ||
| if [ "${FUSE_CONCURRENCY_STRESS_REQUIRED}" = "1" ]; then | ||
| check_required "FUSE concurrency stress" "${{ steps.fuse-concurrency-stress.outcome }}" | ||
| check_required "FUSE concurrency stress" "${{ steps.fuse-concurrency-stress.conclusion }}" | ||
| fi | ||
| if [ "${RUN_FUSE_POSIX_FSX}" = "1" ]; then | ||
| check_required "FUSE POSIX/fsx gate" "${{ steps.fuse-posix-fsx.outcome }}" | ||
| check_required "FUSE POSIX/fsx gate" "${{ steps.fuse-posix-fsx.conclusion }}" | ||
| fi | ||
| if [ "${RUN_E2E_SMOKE_ALL}" = "1" ]; then | ||
| check_required "Full smoke-all suite" "${{ steps.smoke-all.outcome }}" | ||
| check_required "Full smoke-all suite" "${{ steps.smoke-all.conclusion }}" | ||
| fi | ||
| if [ "${RUN_GIT_FEATURE_MATRIX}" = "1" ]; then | ||
| check_required "Git feature matrix" "${{ steps.git-feature-matrix.outcome }}" | ||
| check_required "Git feature matrix" "${{ steps.git-feature-matrix.conclusion }}" | ||
| fi |
1. Restore fast_worktree scenario: GIT_WORKSPACE_SCENARIOS was narrowed to agent_edit_add_commit only, dropping the only e2e coverage for drive9 git worktree add/remove --fast. Add it back. 2. Use steps.<id>.outcome instead of steps.<id>.conclusion in both the summary table and enforcement step. For continue-on-error steps, conclusion is coerced to success even when the step command failed, hiding real failures. outcome reflects the actual step result. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Code Review: Bound full smoke Git workspace workload
Overview
Fixes a CI hang introduced after #519 merged: the full smoke-all step was running an unbounded Git workspace workload. Changes add a 35-minute step timeout, pin Git workspace smoke to a single repo (drive9) and scenario set (agent_edit_add_commit,fast_worktree), tighten clone/mount-ready timeouts, and add --foreground to FUSE mount calls in the feature matrix and git workspace smoke scripts.
Strengths
- Root cause addressed: scoping to one repo + bounded scenarios is the right surgical fix.
- 35-minute hard timeout prevents future CI stalls.
- return 1 fix in _feature-matrix-runner.sh: bare return in a function that uses set -e was masking mount failures with a success exit code. Now correctly propagates failure.
- --foreground on FUSE mounts: keeps the mount process as a child so its stderr lands in the log and process lifecycle is tied to the test. Correct pattern for CI.
- stop_mount shift fix: the old code didn't consume mount_point from $@ before passing remaining args to drive9 umount, so extra args like --pack would be passed in the wrong position. Now correct.
Issues
1. GIT_WORKSPACE_SCENARIOS includes fast_worktree which is not in the PR description
The PR description says scope is pinned to agent_edit_add_commit, but the diff sets:
GIT_WORKSPACE_SCENARIOS: "agent_edit_add_commit,fast_worktree"
If fast_worktree is a heavier scenario, it may partially defeat the bounding intent. Confirm it fits comfortably within the 35-minute budget alongside agent_edit_add_commit.
2. outcome vs conclusion rename in check_required
The variable is renamed from outcome to conclusion. GitHub Actions has different semantics for steps.X.outcome vs steps.X.conclusion (outcome reflects continue-on-error; conclusion does not). Confirm the callers are passing the correct expression -- if they still pass steps.X.outcome this rename is cosmetic but misleading.
3. MOUNT_READY_TIMEOUT_S: "60" may be too tight on cold EC2 runners
If FUSE mount startup takes more than 60 seconds on a cold runner, the smoke step will fail spuriously. Confirm this is based on observed startup times or bump to 90-120 seconds.
Minor
- The restore_pack_archive variable and --pack/--unpack args in run_restore_suite are a functional change bundled into what is described as a CI timeout fix. These look correct but should be called out explicitly in the PR description.
- ignored-build -> build directory rename: same concern -- functional change bundled with CI fix.
Verdict
The timeout and scoping fix is correct and necessary. Confirm fast_worktree scenario is intentional and fits the budget. The two bundled functional changes (pack/unpack in restore suite, directory rename) should be documented in the PR description.
Summary
Run full smoke-all suitewith a 35 minute step timeoutdrive9repo andagent_edit_add_commitscenarioValidation
ruby -e 'require "yaml"; YAML.load_file(".github/workflows/local-e2e.yml")'\n-bash -n e2e/git-workspace-smoke-test.sh e2e/smoke-all.sh\n-git diff --check origin/main...HEAD\n\nFollow-up for PR Fail FUSE performance compare regressions #519 full-gate run 27144169948 hanging in smoke-all after PR Fail FUSE performance compare regressions #519 merged.Summary by CodeRabbit