foundationdb: fix test hang - #131
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/foundationdb/build.ncl`:
- Around line 91-95: The current setup uses fixed paths derived from BASE=$(pwd)
(CLUSTER_FILE, DATA_DIR, LOG_DIR) which can cause cross-run collisions; change
BASE to a unique per-run directory (e.g. create a temp run dir using mktemp -d
or append a timestamp/UUID to $(pwd)) and update CLUSTER_FILE, DATA_DIR, LOG_DIR
to point inside that per-run BASE, then keep the mkdir -p for DATA_DIR and
LOG_DIR (and optionally add a cleanup/trap to remove the per-run dir). Ensure
you update references to BASE, CLUSTER_FILE, DATA_DIR, and LOG_DIR so all
components use the isolated run directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e7b38d-b3e1-48ea-8470-e077ff670cf1
📒 Files selected for processing (1)
packages/foundationdb/build.ncl
| BASE=$(pwd) | ||
| CLUSTER_FILE="$BASE/fdb.cluster" | ||
| DATA_DIR="$BASE/data" | ||
| LOG_DIR="$BASE/logs" | ||
| mkdir -p "$DATA_DIR" "$LOG_DIR" |
There was a problem hiding this comment.
Use a per-run subdirectory under pwd to prevent cross-run state collisions.
Using fixed paths ($PWD/data, $PWD/logs, $PWD/fdb.cluster) can leave stale state and cause flaky reruns or interference when tests share a workdir. You can keep the ext4 workaround and still isolate by creating a unique dir under cwd.
Suggested change
- BASE=$(pwd)
+ BASE=$(mktemp -d -p "$(pwd)" fdb-smoke.XXXXXX)
+ trap 'if [ -n "${SERVER_PID:-}" ]; then kill "$SERVER_PID" 2>/dev/null || true; wait "$SERVER_PID" 2>/dev/null || true; fi; rm -rf "$BASE"' EXIT
CLUSTER_FILE="$BASE/fdb.cluster"
DATA_DIR="$BASE/data"
LOG_DIR="$BASE/logs"
mkdir -p "$DATA_DIR" "$LOG_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/foundationdb/build.ncl` around lines 91 - 95, The current setup uses
fixed paths derived from BASE=$(pwd) (CLUSTER_FILE, DATA_DIR, LOG_DIR) which can
cause cross-run collisions; change BASE to a unique per-run directory (e.g.
create a temp run dir using mktemp -d or append a timestamp/UUID to $(pwd)) and
update CLUSTER_FILE, DATA_DIR, LOG_DIR to point inside that per-run BASE, then
keep the mkdir -p for DATA_DIR and LOG_DIR (and optionally add a cleanup/trap to
remove the per-run dir). Ensure you update references to BASE, CLUSTER_FILE,
DATA_DIR, and LOG_DIR so all components use the isolated run directory.
There was a problem hiding this comment.
these tests are run in an ephemeral sandbox. Not an issue.
There was a problem hiding this comment.
@msample, understood — thanks for the clarification! Since each test run gets its own ephemeral sandbox, fixed paths under $(pwd) are perfectly safe here and there's no risk of cross-run interference. I'll keep this in mind for future reviews.
(´｡• ω •｡`)
✏️ Learnings added
Learnt from: msample
Repo: gominimal/pkgs PR: 131
File: packages/foundationdb/build.ncl:91-95
Timestamp: 2026-04-28T01:52:37.341Z
Learning: In the gominimal/pkgs repository, tests (including those in packages/foundationdb/build.ncl) are run inside ephemeral sandboxes, so using fixed paths under $(pwd) for test state (cluster files, data dirs, log dirs) does not cause cross-run collisions. Isolation concerns about fixed working-directory paths in tests do not apply here.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: gominimal/pkgs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-24T19:04:09.256Z
Learning: Create a new package by creating a 'packages/<name>' directory with at minimum: build.ncl with name, build_deps, cmd, and outputs; build.sh with build instructions
Learnt from: CR
Repo: gominimal/pkgs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-24T19:04:09.256Z
Learning: Applies to packages/**/build.ncl : Declare build scripts as a Local dependency in build_deps: '{ file = "build.sh" } | Local'
For unknown reasons the cluster fails to start on one of my machines when setup on a tmpfs. This adjusts it to run on cwd, which is always on the main fs, which should be backed by ext4.
Summary by CodeRabbit
Note: This is an internal testing infrastructure change with no impact on end-user functionality.