[WIP] offline-replay: resolve OBJDIR from the built binary - #11291
Open
kbhargava-jump wants to merge 2 commits into
Open
[WIP] offline-replay: resolve OBJDIR from the built binary#11291kbhargava-jump wants to merge 2 commits into
kbhargava-jump wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Generic backtests can silently select an arbitrary build flavor when multiple binaries exist.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates offline replay scripts to infer OBJDIR from an existing firedancer-dev binary.
Changes:
- Searches
build/forfiredancer-dev. - Adds executable validation and clearer failure messages.
File summaries
| File | Description |
|---|---|
src/flamenco/runtime/tests/run_ledger_backtest.sh |
Infers and validates OBJDIR. |
contrib/offline-replay/run_offline_replay_backtest.sh |
Resolves the post-build binary location. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Performance Measurements ⏳
|
ripatel-fd
requested changes
Sep 4, 2026
ripatel-fd
left a comment
ripatel-fd
left a comment
Contributor
There was a problem hiding this comment.
This is nondeterministic, should resolve it from the build config, not grab a random firedancer-dev bin
kbhargava-jump
changed the title
kbhargava-jump
force-pushed
the
kbhargava/offline-replay-objdir-fix
branch
from
September 4, 2026 17:41
dbc2c90 to
d51a031
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The checks break download-only mode and can overlook a failed parallel build.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
Performance Measurements ⏳
|
kbhargava-jump
force-pushed
the
kbhargava/offline-replay-objdir-fix
branch
from
September 4, 2026 17:52
d51a031 to
aa7b678
Compare
kbhargava-jump
force-pushed
the
kbhargava/offline-replay-objdir-fix
branch
from
September 4, 2026 17:53
aa7b678 to
450ae83
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new local failure cleanup can either silently fail or remove unrelated host configuration and cached genesis data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+330
to
+332
| if [ -z "$CI" ]; then | ||
| $OBJDIR/bin/firedancer-dev configure fini all --config ${DUMP_DIR}/${LEDGER}_backtest.toml &> /dev/null || true | ||
| fi |
Performance Measurements ⏳
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new general backtest teardown deletes the cached ledger genesis file, breaking subsequent runs.
Review details
Suppressed comments (1)
src/flamenco/runtime/tests/run_ledger_backtest.sh:331
backtestonly initializeshugetlbfs,snapshots, andkeys, butfini allalso runs the genesis stage, whosefiniunlinks the configured$DUMP/$LEDGER/genesis.bin. After a failed local run, the cached ledger directory remains, so the next invocation skips the download and then fails with its genesis missing. Restrict teardown to the stages initialized by backtest; the privileged hugetlbfs teardown also needssudo.
$OBJDIR/bin/firedancer-dev configure fini all --config ${DUMP_DIR}/${LEDGER}_backtest.toml &> /dev/null || true
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
kbhargava-jump
force-pushed
the
kbhargava/offline-replay-objdir-fix
branch
from
September 4, 2026 19:06
450ae83 to
04a4e35
Compare
Performance Measurements ⏳
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The local cleanup silently fails without the privileges required by configure fini all.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| fi | ||
|
|
||
| if [ -z "$CI" ] && grep -q "Unable to set the thread affinity" "$LOG"; then | ||
| $OBJDIR/bin/firedancer-dev configure fini all --config ${DUMP_DIR}/${LEDGER}_backtest.toml &> /dev/null || true |
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
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.
No description provided.