test: deactivate the simulation execution for now#709
Merged
Conversation
Contributor
WalkthroughThe main simulator binary has been modified to disable the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
simulation/amaru-sim/src/bin/amaru-sim/main.rs (2)
30-31: Righto, FIXME could use a bit more meat on the bones, mate.The FIXME mentions the termination isn't reliable and "might stop too soon," but based on the context from
nodes.rs, it looks like the actual issue is the opposite—the drain phase can hang indefinitely if nodes keep generating effects or if the patience threshold (1,000 steps without effects) is never reached. Like a game of whack-a-mole where the moles never stop popping up, yeah?Consider beefing up this comment to point at the specific root cause (e.g., "drain phase in Nodes::run() can hang when effects don't settle") so future you (or your teammates) know exactly what beast needs slaying.
💬 Suggested enhancement to the FIXME
-// FIXME: fix the simulation termination. For now it is not reliable and might stop too soon. +// FIXME: fix the simulation termination. The drain phase in Nodes::run() doesn't reliably +// determine when nodes should stop—it can hang if effects don't settle within the patience +// window (1000 steps) or exceed max drain steps (1M). Needs investigation into why nodes +// continue generating effects indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs` around lines 30 - 31, The FIXME is vague—update it to state that the simulation termination bug is caused by the drain phase in Nodes::run() potentially hanging indefinitely when nodes keep producing effects or the patience threshold (currently 1_000 steps without effects) is never reached; suggest investigating and fixing Nodes::run()'s drain logic (e.g., cap retries, detect quiescence more robustly, or introduce a hard timeout) so future readers know the specific subsystem and variables to inspect.
34-37: Fair dinkum, this binary's gone full stealth mode—no warnings, no breadcrumbs.The simulation's completely disabled but there's zero runtime feedback for anyone running
amaru-sim. They'll just get silence and a clean exit, like a ghost town in a Western. That's gonna confuse folks expecting tests to actually, y'know, run.Since the tracing setup's still there anyway, why not drop a cheeky warning log to let people know the sim's temporarily benched? Think of it as leaving a "gone fishing" sign on the door.
🔔 Suggested runtime warning
tracing_subscriber::fmt() .json() .with_env_filter(EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"))) .init(); + tracing::warn!("Simulation tests are currently disabled due to unreliable termination behavior (see FIXME below)"); + // FIXME: fix the simulation termination. For now it is not reliable and might stop too soon.Also, if you want to track this work properly (issues, milestones, the whole shebang), I can help you draft an issue to document the termination problem and link it to the FIXME. Just give me the nod!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs` around lines 34 - 37, The commented-out test invocation (run_tests) in main has been removed silently; update main (the main function) to emit a runtime warning with the tracing subsystem before exiting so users know tests are intentionally disabled—use tracing::warn! (or the project's tracing logger) to log a clear message like "amaru-sim: tests disabled (FIXME: re-enable or document)" referencing the existing FIXME, and keep the run_tests symbol in the message so it's obvious what’s been disabled; do not change control flow beyond logging (i.e., exit as before) and ensure the log happens after tracing is initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs`:
- Around line 30-31: The FIXME is vague—update it to state that the simulation
termination bug is caused by the drain phase in Nodes::run() potentially hanging
indefinitely when nodes keep producing effects or the patience threshold
(currently 1_000 steps without effects) is never reached; suggest investigating
and fixing Nodes::run()'s drain logic (e.g., cap retries, detect quiescence more
robustly, or introduce a hard timeout) so future readers know the specific
subsystem and variables to inspect.
- Around line 34-37: The commented-out test invocation (run_tests) in main has
been removed silently; update main (the main function) to emit a runtime warning
with the tracing subsystem before exiting so users know tests are intentionally
disabled—use tracing::warn! (or the project's tracing logger) to log a clear
message like "amaru-sim: tests disabled (FIXME: re-enable or document)"
referencing the existing FIXME, and keep the run_tests symbol in the message so
it's obvious what’s been disabled; do not change control flow beyond logging
(i.e., exit as before) and ensure the log happens after tracing is initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fd59276-4718-4914-870d-711ca64ddf9c
📒 Files selected for processing (1)
simulation/amaru-sim/src/bin/amaru-sim/main.rs
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
etorreborre
force-pushed
the
etorreborre/test/deactivate-simulation
branch
from
1324451 to
476f566
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
simulation/amaru-sim/src/bin/amaru-sim/main.rs (1)
30-30: Please tie this FIXME to a tracked issue.Nice heads-up comment, but Line 30 is currently an open-ended side quest. Add an issue/ID so it doesn’t become permanent “temporary” code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs` at line 30, Replace the open-ended FIXME comment in main (the line containing "// FIXME: fix the simulation termination...") with a tracked-issue reference by updating it to include the issue ID and short status, e.g. "// FIXME (ISSUE-1234): fix the simulation termination — tracked at ISSUE-1234", and if applicable add a brief TODO note pointing to the issue URL; ensure the comment references the same symbol (main / simulation termination) so future readers can find the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@justfile`:
- Around line 13-15: The docker run in the end-to-end step hard-codes host temp
mounts and a fixed container name which breaks local runs and reruns; change the
cardano-node run invocation to use a configurable/derived host temp directory
(e.g., an env var like PREPROD_DB_DIR or $RUNNER_TEMP with a fallback) instead
of the literal /home/runner/work/_temp/... path and make the container lifecycle
idempotent by either using --rm, appending a unique suffix to --name (e.g.,
cardano-node-$CI_JOB_ID) or explicitly cleaning any existing container (docker
rm -f cardano-node) before starting; update the justfile docker run line and any
related download/start helpers to reference these variables (look for the docker
run command and the literal container name "cardano-node" in the diff).
In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs`:
- Around line 34-37: Remove or fix the temporary disabled simulation artifacts:
either delete the unused run_tests import (remove the use/import of run_tests)
if you keep the simulation code commented out, or re-enable the run_tests call
and handle its Result by logging errors (use process-friendly logging rather
than silently exiting) if you intend to restore execution; also mark the unused
CLI args as intentionally unused by renaming args to _args (or prefixing with an
underscore) so the compiler stops warning. Ensure you update the FIXME
accordingly when you re-enable run_tests and replace silent exits with a clear
error log.
---
Nitpick comments:
In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs`:
- Line 30: Replace the open-ended FIXME comment in main (the line containing "//
FIXME: fix the simulation termination...") with a tracked-issue reference by
updating it to include the issue ID and short status, e.g. "// FIXME
(ISSUE-1234): fix the simulation termination — tracked at ISSUE-1234", and if
applicable add a brief TODO note pointing to the issue URL; ensure the comment
references the same symbol (main / simulation termination) so future readers can
find the issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30f5d6b3-3023-4d65-90e3-b56d0dca6616
📒 Files selected for processing (2)
justfilesimulation/amaru-sim/src/bin/amaru-sim/main.rs
justfile
Outdated
Comment on lines
+13
to
+15
| docker pull ghcr.io/intersectmbo/cardano-node:10.1.4 | ||
| make HASKELL_NODE_CONFIG_DIR=cardano-node-config NETWORK=preprod download-haskell-config | ||
| docker run --platform=linux/amd64 -d --name cardano-node -v /home/runner/work/_temp/db-preprod:/db -v /home/runner/work/_temp/ipc:/ipc -v $PWD/cardano-node-config:/config -v $PWD/cardano-node-config:/genesis -p 3001:3001 ghcr.io/intersectmbo/cardano-node:10.1.4 run --config /config/config.json --database-path /db --socket-path /ipc/node.socket --topology /config/topology.json |
Contributor
There was a problem hiding this comment.
end-to-end is currently CI-host-path-coupled and brittle on reruns.
Line 15 hard-codes /home/runner/work/_temp/..., so local runs can break. Also, fixed --name cardano-node without pre-cleanup can fail on second run.
🛠️ Suggested portability/retry patch
end-to-end:
docker pull ghcr.io/intersectmbo/cardano-node:10.1.4
make HASKELL_NODE_CONFIG_DIR=cardano-node-config NETWORK=preprod download-haskell-config
- docker run --platform=linux/amd64 -d --name cardano-node -v /home/runner/work/_temp/db-preprod:/db -v /home/runner/work/_temp/ipc:/ipc -v $PWD/cardano-node-config:/config -v $PWD/cardano-node-config:/genesis -p 3001:3001 ghcr.io/intersectmbo/cardano-node:10.1.4 run --config /config/config.json --database-path /db --socket-path /ipc/node.socket --topology /config/topology.json
+ docker rm -f cardano-node >/dev/null 2>&1 || true
+ docker run --platform=linux/amd64 -d --name cardano-node -v ${TMPDIR:-/tmp}/db-preprod:/db -v ${TMPDIR:-/tmp}/ipc:/ipc -v $PWD/cardano-node-config:/config -v $PWD/cardano-node-config:/genesis -p 3001:3001 ghcr.io/intersectmbo/cardano-node:10.1.4 run --config /config/config.json --database-path /db --socket-path /ipc/node.socket --topology /config/topology.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker pull ghcr.io/intersectmbo/cardano-node:10.1.4 | |
| make HASKELL_NODE_CONFIG_DIR=cardano-node-config NETWORK=preprod download-haskell-config | |
| docker run --platform=linux/amd64 -d --name cardano-node -v /home/runner/work/_temp/db-preprod:/db -v /home/runner/work/_temp/ipc:/ipc -v $PWD/cardano-node-config:/config -v $PWD/cardano-node-config:/genesis -p 3001:3001 ghcr.io/intersectmbo/cardano-node:10.1.4 run --config /config/config.json --database-path /db --socket-path /ipc/node.socket --topology /config/topology.json | |
| docker pull ghcr.io/intersectmbo/cardano-node:10.1.4 | |
| make HASKELL_NODE_CONFIG_DIR=cardano-node-config NETWORK=preprod download-haskell-config | |
| docker rm -f cardano-node >/dev/null 2>&1 || true | |
| docker run --platform=linux/amd64 -d --name cardano-node -v ${TMPDIR:-/tmp}/db-preprod:/db -v ${TMPDIR:-/tmp}/ipc:/ipc -v $PWD/cardano-node-config:/config -v $PWD/cardano-node-config:/genesis -p 3001:3001 ghcr.io/intersectmbo/cardano-node:10.1.4 run --config /config/config.json --database-path /db --socket-path /ipc/node.socket --topology /config/topology.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@justfile` around lines 13 - 15, The docker run in the end-to-end step
hard-codes host temp mounts and a fixed container name which breaks local runs
and reruns; change the cardano-node run invocation to use a configurable/derived
host temp directory (e.g., an env var like PREPROD_DB_DIR or $RUNNER_TEMP with a
fallback) instead of the literal /home/runner/work/_temp/... path and make the
container lifecycle idempotent by either using --rm, appending a unique suffix
to --name (e.g., cardano-node-$CI_JOB_ID) or explicitly cleaning any existing
container (docker rm -f cardano-node) before starting; update the justfile
docker run line and any related download/start helpers to reference these
variables (look for the docker run command and the literal container name
"cardano-node" in the diff).
476f566 to
366ea3f
Compare
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/test/deactivate-simulation
branch
from
366ea3f to
08a4ff3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
simulation/amaru-sim/src/bin/amaru-sim/main.rs (1)
28-35: Fair dinkum, the FIXME tells the story — but anyone checking the CI logs is gonna see sweet FA about why no tests ran. It's like finishing Dark Souls but the credits never roll... did I even play?Reckon a wee
tracing::warn!would be a ripper here so the nightly workflow logs make it crystal clear the simulation is having a kip:💡 Suggested enhancement
// FIXME: fix the simulation termination. For now it is not reliable and might stop too soon. + tracing::warn!("Simulation tests are currently disabled pending termination fixes"); // It might be necessary to run the simulation with a larger stack with RUST_MIN_STACK=16777216 (16MB)This way, when someone's poking through the nightly-simulation workflow logs wondering why artifacts weren't uploaded on failure, they'll see exactly what's going on instead of a ghost town of silence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs` around lines 28 - 35, The FIXME block in main.rs currently silences test execution and provides no CI-visible feedback; update the code around the commented run_tests handling to emit a clear tracing::warn! explaining that simulation termination is unreliable and tests are skipped (include the FIXME tag text and any relevant args), ensure tracing is imported/initialized before use (e.g., where main initializes logging), and keep the commented-out run_tests call so future restoration is obvious; target the block containing the commented run_tests/exit handling so CI logs show the warning when simulations are not run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs`:
- Around line 28-35: The FIXME block in main.rs currently silences test
execution and provides no CI-visible feedback; update the code around the
commented run_tests handling to emit a clear tracing::warn! explaining that
simulation termination is unreliable and tests are skipped (include the FIXME
tag text and any relevant args), ensure tracing is imported/initialized before
use (e.g., where main initializes logging), and keep the commented-out run_tests
call so future restoration is obvious; target the block containing the commented
run_tests/exit handling so CI logs show the warning when simulations are not
run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89e54e9c-1c44-4a3f-8ba2-5b1459e05052
📒 Files selected for processing (1)
simulation/amaru-sim/src/bin/amaru-sim/main.rs
jeluard
approved these changes
Mar 6, 2026
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
2 participants
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.
It's not reliably determining when the nodes should stop to run. We need to come back to this.
Summary by CodeRabbit
Bug Fixes
Chores