feat: add DST setup #879
feat: add DST setup #879
Conversation
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
WalkthroughThis set of changes introduces new build and workload automation for Antithesis testing, adds several Go programs for workload orchestration and validation, and enhances documentation and error handling in the ledger creation process. New Earthfiles and configuration scripts are provided to support multi-stage builds, containerization, and experiment launching on the Antithesis platform. Utility functions are introduced to facilitate ledger operations, random data generation, and assertions during workload execution. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Antithesis Earthfile
participant Antithesis API
participant Build Targets
participant Workload Binary
User->>Antithesis Earthfile: Trigger `run` or `run-1h` target
Antithesis Earthfile->>Build Targets: Build config, image, workload
Build Targets-->>Antithesis Earthfile: Build artifacts/images
Antithesis Earthfile->>Antithesis API: POST /api/v1/launch_experiment/formance
Antithesis API-->>Antithesis Earthfile: Experiment launched
Antithesis API->>Workload Binary: Start workload containers
Workload Binary->>Ledger Service: Run workload logic (create ledger, submit tx, etc.)
sequenceDiagram
participant Workload Binary
participant Utility Functions
participant Ledger Client
participant Ledger Service
Workload Binary->>Utility Functions: Call (e.g., CreateLedger, RunTx)
Utility Functions->>Ledger Client: API call (create ledger, submit tx, list ledgers)
Ledger Client->>Ledger Service: HTTP request
Ledger Service-->>Ledger Client: Response
Ledger Client-->>Utility Functions: Return result
Utility Functions-->>Workload Binary: Return result
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #879 +/- ##
==========================================
- Coverage 81.97% 81.74% -0.24%
==========================================
Files 141 141
Lines 7746 7762 +16
==========================================
- Hits 6350 6345 -5
- Misses 1070 1088 +18
- Partials 326 329 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
test/antithesis/workload/bin/init/main.go (1)
28-34: Consider adding a log message for setup completion and shutdown.Adding log messages would make it easier to understand the program flow, especially when running in containerized environments where logs are a primary debugging tool.
lifecycle.SetupComplete(map[string]any{}) + fmt.Println("Setup complete, waiting for termination signal") sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) <-sigs + fmt.Println("Received termination signal, shutting down") }test/antithesis/image/entrypoint.sh (1)
11-11: Add proper signal handling for container graceful shutdown.The script should properly handle termination signals to ensure clean shutdown of the ledger service.
echo "Postgres is ready; serving ledger!" +# Handle termination signals +trap 'echo "Received termination signal. Shutting down..."; kill -TERM $child_pid 2>/dev/null; exit 0' SIGTERM SIGINT + ledger serve +child_pid=$! +wait $child_pidtest/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (3)
69-71: Improve error handling consistency.The error handling here doesn't provide specific error details in the assertion, unlike other error checks in the code.
- if !internal.AssertAlwaysErrNil(err, "we should be able to query account 'world'", nil) { + if !internal.AssertAlwaysErrNil(err, "we should be able to query account 'world'", internal.Details{ + "error": err, + "ledger": ledger, + }) { return }
19-31: Consider adding ledger cleanup after test completion.The code creates a new ledger for each test run but doesn't clean it up afterward, which could lead to resource accumulation over time.
Add a defer statement after the ledger creation:
ledger := fmt.Sprintf("ledger-%d", internal.RandomBigInt().Int64()) + // Ensure ledger cleanup after test + defer func() { + cleanupErr := internal.DeleteLedger(ctx, client, ledger) + if cleanupErr != nil { + fmt.Printf("Warning: Failed to clean up ledger %s: %v\n", ledger, cleanupErr) + } + }() + err := internal.CreateLedger(Note: This assumes there's a
DeleteLedgerfunction in the internal package. If not, you would need to implement it.
58-61: Simplify conditional check.The
condvariable is only used once, making it unnecessary.- cond := !hasError.Load() - if assert.Always(cond, "all transactions should have been written", internal.Details{}); !cond { + if assert.Always(!hasError.Load(), "all transactions should have been written", internal.Details{}); hasError.Load() { return }test/antithesis/workload/Earthfile (3)
43-45: Add filtering for Go files in the build loop.The current loop builds all directories in bin/cmds without filtering. If there are non-Go files or directories without main packages, this could cause build failures.
- FOR file IN $(ls bin/cmds) - RUN go build -o /out/cmds/$file ./bin/cmds/$file + FOR file IN $(find bin/cmds -type d -not -path "bin/cmds" | xargs -n1 basename) + # Check if the directory contains a main.go file before building + RUN if [ -f ./bin/cmds/$file/main.go ]; then \ + go build -o /out/cmds/$file ./bin/cmds/$file; \ + fi END
56-56: Consider providing documentation for the required ANTITHESIS_REPOSITORY argument.The ANTITHESIS_REPOSITORY argument is required but has no documentation or validation on format.
- ARG --required ANTITHESIS_REPOSITORY + # ANTITHESIS_REPOSITORY should be in the format of "registry.example.com/project" + ARG --required ANTITHESIS_REPOSITORY + # Validate repository format + RUN if ! echo "${ANTITHESIS_REPOSITORY}" | grep -q "^[a-zA-Z0-9][a-zA-Z0-9.-]*\.[a-zA-Z0-9][a-zA-Z0-9.-]*/[a-zA-Z0-9_/-]*$"; then \ + echo "Invalid repository format: ${ANTITHESIS_REPOSITORY}"; \ + echo "Expected format: registry.example.com/project"; \ + exit 1; \ + fi
15-26: Consider uncommenting the 'tidy' section or add a comment explaining why it's commented out.The 'tidy' section is commented out but might be useful for maintaining clean go.mod files.
Either uncomment the section if it's meant to be used, or add a comment explaining why it's disabled:
-# tidy: -# FROM +sources -# -# CACHE --id go-mod-cache /go/pkg/mod -# CACHE --id go-cache /root/.cache/go-build -# -# WORKDIR /src/test/antithesis/workload -# RUN go mod tidy -# -# SAVE ARTIFACT go.mod AS LOCAL go.mod -# SAVE ARTIFACT go.sum AS LOCAL go.sum +# The 'tidy' target is currently disabled because module maintenance is handled in the main repository. +# Uncomment and use this section when updates to go.mod/go.sum are needed: +# tidy: +# FROM +sources +# +# CACHE --id go-mod-cache /go/pkg/mod +# CACHE --id go-cache /root/.cache/go-build +# +# WORKDIR /src/test/antithesis/workload +# RUN go mod tidy +# +# SAVE ARTIFACT go.mod AS LOCAL go.mod +# SAVE ARTIFACT go.sum AS LOCAL go.sumtest/antithesis/Earthfile (1)
7-26: Review experiment parametersThe experiment duration is set to
0.1, which might be very short for meaningful testing. Consider whether this is intended as a minimum value for quick tests or if a longer duration would be more appropriate for thorough testing.Also verify that the fixed image names (
antithesis-config:latest,ledger:latest,workload:latest) match your intended tagging strategy.test/antithesis/workload/internal/utils.go (2)
40-56: Hardcoded service endpointThe client is configured with a hardcoded URL
http://gateway:8080. Consider making this configurable via environment variables for better flexibility across different environments.+func NewClientWithURL(serverURL string) *client.Formance { + if serverURL == "" { + serverURL = "http://gateway:8080" // Default URL + } + return client.New( + client.WithServerURL(serverURL), + client.WithClient(&http.Client{ + Timeout: time.Minute, + }), + client.WithRetryConfig(retry.Config{ + Strategy: "backoff", + Backoff: &retry.BackoffStrategy{ + InitialInterval: 200, + Exponent: 1.5, + MaxElapsedTime: 4000, + }, + RetryConnectionErrors: true, + }), + ) +} + func NewClient() *client.Formance { - return client.New( - client.WithServerURL("http://gateway:8080"), - client.WithClient(&http.Client{ - Timeout: time.Minute, - }), - client.WithRetryConfig(retry.Config{ - Strategy: "backoff", - Backoff: &retry.BackoffStrategy{ - InitialInterval: 200, - Exponent: 1.5, - MaxElapsedTime: 4000, - }, - RetryConnectionErrors: true, - }), - ) + return NewClientWithURL("") }
66-82: Transaction function with hardcoded valuesThe
RunTxfunction uses hardcoded values for asset ("USD/2") and source account ("world"). Consider making these configurable parameters to support testing different asset types and account structures.-func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string) error { +func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string, asset string, source string) error { + if asset == "" { + asset = "USD/2" // Default asset + } + if source == "" { + source = "world" // Default source + } orderID := fmt.Sprint(int64(math.Abs(float64(random.GetRandom())))) dest := fmt.Sprintf("orders:%s", orderID) _, err := client.Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ V2PostTransaction: components.V2PostTransaction{ Postings: []components.V2Posting{{ Amount: amount, - Asset: "USD/2", + Asset: asset, Destination: dest, - Source: "world", + Source: source, }}, }, Ledger: ledger, }) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/antithesis.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtest/antithesis/config/docker-compose.ymlis excluded by!**/*.ymltest/antithesis/workload/go.modis excluded by!**/*.modtest/antithesis/workload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
internal/controller/system/controller.go(1 hunks)internal/storage/driver/driver.go(4 hunks)pkg/client/Earthfile(1 hunks)test/antithesis/Earthfile(1 hunks)test/antithesis/config/Earthfile(1 hunks)test/antithesis/config/gateway/Caddyfile(1 hunks)test/antithesis/image/Earthfile(1 hunks)test/antithesis/image/entrypoint.sh(1 hunks)test/antithesis/workload/Earthfile(1 hunks)test/antithesis/workload/bin/cmds/eventually_aggregated_volumes_should_be_zeroed/main.go(1 hunks)test/antithesis/workload/bi 8000 n/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/bin/init/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/antithesis/workload/bin/init/main.go (2)
test/antithesis/workload/internal/utils.go (1)
NewClient(40-56)internal/api/v1/controllers_config.go (1)
GetInfo(36-66)
test/antithesis/workload/internal/utils.go (5)
pkg/client/formance.go (3)
Formance(70-74)WithClient(108-112)WithRetryConfig(130-134)pkg/client/models/operations/v2createledger.go (1)
V2CreateLedgerRequest(9-13)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-23)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(29-36)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)
🪛 GitHub Check: codecov/patch
internal/storage/driver/driver.go
[warning] 84-95: internal/storage/driver/driver.go#L84-L95
Added lines #L84 - L95 were not covered by tests
🔇 Additional comments (15)
internal/controller/system/controller.go (1)
4-7: Import reordering looks good.The standard library packages
reflectandtimehave been moved to the top of the import block, following common Go style conventions where standard library imports are grouped together at the top.pkg/client/Earthfile (1)
1-8: Clean Earthfile setup for the client component.This is a good minimal Earthfile that creates a client artifact from the current directory. Using a scratch image keeps the build artifact lightweight. The golang version (1.22.2) is appropriately recent.
test/antithesis/config/gateway/Caddyfile (1)
1-12: Appropriate reverse proxy configuration for local development.The Caddy configuration correctly sets up a reverse proxy to ledger services. The
auto_https offanddebugoptions are suitable for a development environment. The load balancing policy of "first" will try the first available backend before attempting the second one.Just note that this configuration is explicitly for development and would need security enhancements (enabling HTTPS, etc.) for any production deployment.
test/antithesis/config/Earthfile (1)
1-11: Straightforward config build setup.The Earthfile correctly configures a build process to create a config image containing the docker-compose file and gateway directory. The use of
--no-manifest-listis appropriate when targeting a specific platform.test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (1)
35-43: 🛠️ Refactor suggestionFix potential race condition with totalAmount.
There's a potential race condition in how you're calculating the total amount. While the code is adding to totalAmount before submitting the tasks to the pool, this pattern is prone to bugs if the code is modified later.
hasError := atomic.NewBool(false) - totalAmount := big.NewInt(0) + // Track individual amounts for safer aggregation + amounts := make([]*big.Int, count) pool := pond.New(10, 10e3) for i := 0; i < count; i++ { amount := internal.RandomBigInt() - totalAmount = totalAmount.Add(totalAmount, amount) + amounts[i] = amount pool.Submit(func() {Then after line 56 (after pool.StopAndWait()):
pool.StopAndWait() + // Calculate total after all tasks are submitted + totalAmount := big.NewInt(0) + for _, amount := range amounts { + totalAmount = totalAmount.Add(totalAmount, amount) + } + cond := !hasError.Load()Likely an incorrect or invalid review comment.
internal/storage/driver/driver.go (3)
10-10: Great addition of the Antithesis SDKThe import of the Antithesis assert package is appropriate for the new assertion functionality added in this file.
40-53: Excellent documentation improvementAdding this comprehensive doc comment significantly improves code clarity. The step-by-step explanation of the ledger creation process and transaction handling makes the complex functionality much easier to understand for developers.
57-59: Good inline commentsThese explanatory comments help track the progress through the ledger creation process and align with the high-level documentation. This makes the code much more maintainable.
Also applies to: 62-63, 108-109, 114-115, 119-120
test/antithesis/Earthfile (3)
1-6: LGTM - Base configuration setupThe Earthfile version and imports look correct. Using a tagged version of the core Earthly configuration is a good practice for stability.
27-52: Good debugging setupThe debugging target properly handles session parameters and shows them for verification before submission. The approach of creating a temporary JSON file for the parameters is clean and maintainable.
53-58: LGTM - Build dependencies handled correctlyThe requirements-build target properly passes arguments to the component build targets and enforces the required ANTITHESIS_REPOSITORY argument.
test/antithesis/image/Earthfile (1)
7-9: Good cache configurationUsing shared caching for Go modules and build artifacts is an excellent practice that will speed up builds.
test/antithesis/workload/internal/utils.go (3)
19-26: Good utility type and random functionThe
Detailstype andRandomBigIntfunction are well-implemented. The random function correctly converts the random value to a big integer for use in financial transactions.
28-38: Helpful assertion wrappersThese wrapper functions around the Antithesis assertion library provide a consistent interface for making assertions throughout the workload code. The return values allow for fluent usage in conditional blocks.
58-64: Simple, focused ledger creation functionThis function provides a clean wrapper around the client API for creating ledgers, making the workload code more readable.
| func main() { | ||
| ctx := context.Background() | ||
| client := internal.NewClient() | ||
|
|
||
| for { | ||
| time.Sleep(time.Second) | ||
|
|
||
| _, err := client.Ledger.GetInfo(ctx) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| break | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error logging and timeout to service readiness loop.
The polling mechanism to check for service readiness is a good approach, but it currently lacks error logging and a timeout mechanism. Without these, it could poll indefinitely if the service never becomes available, and debugging could be difficult if there are connection issues.
func main() {
ctx := context.Background()
client := internal.NewClient()
+
+ maxAttempts := 60 // 1 minute timeout with 1-second intervals
+ attempt := 0
for {
+ attempt++
time.Sleep(time.Second)
_, err := client.Ledger.GetInfo(ctx)
if err != nil {
+ if attempt >= maxAttempts {
+ panic("Timed out waiting for ledger service to be ready")
+ }
+ fmt.Printf("Waiting for ledger service to be ready (attempt %d/%d): %v\n", attempt, maxAttempts, err)
continue
}
+ fmt.Println("Ledger service is ready")
break
}Don't forget to add the fmt import:
import (
"context"
+ "fmt"
"os"
"os/signal"
"syscall"
"time"📝 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.
| func main() { | |
| ctx := context.Background() | |
| client := internal.NewClient() | |
| for { | |
| time.Sleep(time.Second) | |
| _, err := client.Ledger.GetInfo(ctx) | |
| if err != nil { | |
| continue | |
| } | |
| break | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "os" | |
| "os/signal" | |
| "syscall" | |
| "time" | |
| ) | |
| func main() { | |
| ctx := context.Background() | |
| client := internal.NewClient() | |
| maxAttempts := 60 // 1 minute timeout with 1-second intervals | |
| attempt := 0 | |
| for { | |
| attempt++ | |
| time.Sleep(time.Second) | |
| _, err := client.Ledger.GetInfo(ctx) | |
| if err != nil { | |
| if attempt >= maxAttempts { | |
| panic("Timed out waiting for ledger service to be ready") | |
| } | |
| fmt.Printf("Waiting for ledger service to be ready (attempt %d/%d): %v\n", attempt, maxAttempts, err) | |
| continue | |
| } | |
| fmt.Println("Ledger service is ready") | |
| break | |
| } | |
| // rest of your shutdown or signal handling logic… | |
| } |
| until pg_isready -d ledger -h 10.0.29.16 -U ledger | ||
| do | ||
| echo "Waiting for postgres at: $POSTGRES_URI" | ||
| sleep 2; | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make database connection parameters configurable via environment variables.
The script uses hardcoded database host, name, and user values. This reduces portability and makes configuration changes difficult.
-until pg_isready -d ledger -h 10.0.29.16 -U ledger
+# Extract parameters from POSTGRES_URI or use default values
+PG_HOST=${PG_HOST:-10.0.29.16}
+PG_DB=${PG_DB:-ledger}
+PG_USER=${PG_USER:-ledger}
+
+until pg_isready -d "$PG_DB" -h "$PG_HOST" -U "$PG_USER"📝 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.
| until pg_isready -d ledger -h 10.0.29.16 -U ledger | |
| do | |
| echo "Waiting for postgres at: $POSTGRES_URI" | |
| sleep 2; | |
| done | |
| # Extract parameters from environment or fall back to existing defaults | |
| PG_HOST=${PG_HOST:-10.0.29.16} | |
| PG_DB=${PG_DB:-ledger} | |
| PG_USER=${PG_USER:-ledger} | |
| until pg_isready -d "$PG_DB" -h "$PG_HOST" -U "$PG_USER" | |
| do | |
| echo "Waiting for postgres at: $POSTGRES_URI" | |
| sleep 2; | |
| done |
🛠️ Refactor suggestion
Add a timeout to prevent infinite waiting.
The current loop will wait indefinitely if PostgreSQL never becomes available.
+MAX_RETRIES=30
+retry_count=0
+
until pg_isready -d ledger -h 10.0.29.16 -U ledger
do
+ retry_count=$((retry_count+1))
+ if [ $retry_count -ge $MAX_RETRIES ]; then
+ echo "ERROR: PostgreSQL not available after $MAX_RETRIES attempts. Exiting."
+ exit 1
+ fi
echo "Waiting for postgres at: $POSTGRES_URI"
sleep 2;
done📝 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.
| until pg_isready -d ledger -h 10.0.29.16 -U ledger | |
| do | |
| echo "Waiting for postgres at: $POSTGRES_URI" | |
| sleep 2; | |
| done | |
| MAX_RETRIES=30 | |
| retry_count=0 | |
| until pg_isready -d ledger -h 10.0.29.16 -U ledger | |
| do | |
| retry_count=$((retry_count+1)) | |
| if [ $retry_count -ge $MAX_RETRIES ]; then | |
| echo "ERROR: PostgreSQL not available after $MAX_RETRIES attempts. Exiting." | |
| exit 1 | |
| fi | |
| echo "Waiting for postgres at: $POSTGRES_URI" | |
| sleep 2; | |
| done |
| for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { | ||
| assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{ | ||
| "error": err, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect error reference in assertion details.
In the assertion for each asset's volume, you're referencing the err variable that has already been checked to be nil earlier in the code. This is misleading and could cause confusion.
for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data {
- assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{
- "error": err,
- })
+ assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{
+ "asset": asset,
+ "value": volumes.String(),
+ })
}📝 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.
| for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { | |
| assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{ | |
| "error": err, | |
| }) | |
| } | |
| for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { | |
| assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{ | |
| "asset": asset, | |
| "value": volumes.String(), | |
| }) | |
| } |
| func main() { | ||
| ctx := context.Background() | ||
| client := internal.NewClient() | ||
|
|
||
| aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ | ||
| Ledger: "default", | ||
| }) | ||
| if err != nil { | ||
| assert.Always(err == nil, "error getting aggregated balances", internal.Details{ | ||
| "error": err, | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement retry mechanism for eventual consistency check.
The filename includes "eventually_" which suggests this check should be retried until it succeeds or times out. Currently, it only checks once without any retry logic.
func main() {
ctx := context.Background()
client := internal.NewClient()
+
+ // Add retry with timeout for eventual consistency
+ const maxRetries = 30
+ const retryDelayMs = 500
+
+ var lastErr error
+ for i := 0; i < maxRetries; i++ {
+ success := tryCheck(ctx, client)
+ if success {
+ return // Success case exits cleanly
+ }
+
+ // Wait before retry
+ time.Sleep(time.Duration(retryDelayMs) * time.Millisecond)
+ }
+
+ // If we get here, we've exceeded our retry attempts
+ assert.Always(false, "aggregated volumes not zeroed after maximum retries", internal.Details{})
+}
+
+func tryCheck(ctx context.Context, client *internal.Client) bool {Further down, you would need to modify the end of the function to return a boolean success value instead of just returning:
if err != nil {
assert.Always(err == nil, "error getting aggregated balances", internal.Details{
"error": err,
})
- return
+ return false
}
A93C
for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data {
- assert.Always(volumes.Cmp(new(big.Int)) == 0, "aggregated volumes for asset "+asset+" should be 0", internal.Details{
- "error": err,
- })
+ if volumes.Cmp(new(big.Int)) != 0 {
+ return false
+ }
}
+ return true
}Committable suggestion skipped: line range outside the PR's diff.
| CACHE --id go-mod-cache /go/pkg/mod | ||
| CACHE --id go-cache /root/.cache/go-build | ||
|
|
||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the version of antithesis-go-instrumentor tool.
Using @latest for tools can lead to inconsistent builds and unexpected breaking changes. Consider pinning to a specific version.
- RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest
+ RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@v0.5.2 # Use appropriate version📝 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.
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest | |
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@v0.5.2 # Use appropriate version |
| assert.AlwaysOrUnreachable( | ||
| // @todo: replace this with a proper flag detailing wether we're | ||
| // operating a new version of the binary or not. | ||
| // if we are, we are definitely expecting this to happen. | ||
| // if we're not, this should be unreachable. | ||
| false, | ||
| "Bucket is outdated", | ||
| map[string]any{ | ||
| "bucket": l.Bucket, | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Todo indicates temporary assertion pattern
The assertion is using false as the condition, which means it will always trigger as "unreachable". The TODO comments indicate this is a temporary solution, but this code path isn't covered by tests.
Consider:
- Adding a proper flag parameter as mentioned in the TODO
- Adding tests to cover this code path
🏁 Script executed:
#!/bin/bash
# Check if there are any tests covering this assertion in the driver code
rg -A 5 -B 5 "test.*Bucket.*Outdated" --type goLength of output: 49
To be certain this assertion path is untested, let’s list any driver test files and search them for AlwaysOrUnreachable:
🏁 Script executed:
#!/bin/bash
# List Go test files in internal/storage/driver
fd -e go --glob '*_test.go' internal/storage/driver
# Search those test files for AlwaysOrUnreachable
rg "AlwaysOrUnreachable" -n --glob "internal/storage/driver/*_test.go"Length of output: 348
Ensure proper flag control and add tests for the unreachable assertion
The assert.AlwaysOrUnreachable(false, …) at internal/storage/driver/driver.go:84–95 is built to always fire, and our search shows no tests reference AlwaysOrUnreachable to cover this path. To address:
• Introduce the boolean flag referenced in the TODO (e.g. isNewBinaryVersion) instead of hard‑coding false.
• Add unit tests in internal/storage/driver/driver_test.go (or a new test) that cover both cases:
- Flag =
true(assertion bypassed) - Flag =
false(assertion triggers)
This will satisfy the TODO and ensure the code path is exercised.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 84-95: internal/storage/driver/driver.go#L84-L95
Added lines #L84 - L95 were not covered by tests
| compile: | ||
| FROM --platform=linux/amd64 golang:1.23 | ||
| CACHE --sharing=shared --id go-mod-cache /go/pkg/mod | ||
| CACHE --sharing=shared --id go-cache /root/.cache/go-build | ||
|
|
||
| COPY ../../..+sources/src /src | ||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest | ||
| WORKDIR /src | ||
|
|
||
| RUN go mod download | ||
| RUN mkdir -p /ledger_instrumented | ||
| RUN /go/bin/antithesis-go-instrumentor . /ledger_instrumented | ||
|
|
||
| WORKDIR /ledger_instrumented/customer | ||
| RUN go build -race -o ledger | ||
|
|
||
| SAVE ARTIFACT /ledger_instrumented/customer/ledger | ||
| SAVE ARTIFACT /ledger_instrumented/symbols | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check Go version
The compile stage uses golang:1.23, but the latest stable Go version is 1.22.x. This appears to be a non-existent future version.
- FROM --platform=linux/amd64 golang:1.23
+ FROM --platform=linux/amd64 golang:1.22🌐 Web query:
What is the latest stable version of Go?
💡 Result:
The latest stable version of Go as of April 2025 is Go 1.24.2, released on April 1, 2025[1][2]. This release includes security fixes to the net/http package as well as various bug fixes to the compiler, runtime, go command, and several libraries such as crypto/tls, go/types, net/http, and testing[2].
Citations:
- 1: https://go.dev/dl/
- 2: https://go.dev/doc/devel/release
- 3: https://tip.golang.org/doc/go1.24
- 4: https://github.com/golang/tools/releases
- 5: https://antonz.org/which-go/
- 6: https://tip.golang.org/doc/go1.23
- 7: https://discussion.fedoraproject.org/t/fedora-doesnt-provide-latest-version-of-go/146930
- 8: https://endoflife.date/go
Bump Go base image to latest stable (1.24)
The compile stage currently uses an outdated Go image (golang:1.23), but the latest stable release as of April 2025 is Go 1.24.2. To include the latest security and bug fixes, please update the Dockerfile:
File: test/antithesis/image/Earthfile
Lines: 10–28
- FROM --platform=linux/amd64 golang:1.23
+ FROM --platform=linux/amd64 golang:1.24If you prefer explicit patch‑level pinning, you can use:
FROM --platform=linux/amd64 golang:1.24.2| build: | ||
| FROM --platform=linux/amd64 ubuntu:latest | ||
| RUN apt-get update -y && apt-get install -y postgresql-client curl | ||
|
|
||
| COPY (+compile/ledger) /bin/ledger | ||
| COPY (+compile/symbols) /symbols | ||
| COPY entrypoint.sh /bin/entrypoint.sh | ||
|
|
||
| RUN chmod 777 /bin/entrypoint.sh | ||
|
|
||
| ENTRYPOINT ["/bin/entrypoint.sh"] | ||
| EXPOSE 8080 | ||
|
|
||
| ARG --required ANTITHESIS_REPOSITORY | ||
|
|
||
| SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:latest" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider version tagging strategy
The image is currently saved with the tag ledger:latest. While this works for simple workflows, consider adding a versioning strategy (like using git tags, commits, or timestamps) for better traceability in production environments.
Also verify that the entrypoint script has appropriate error handling for graceful failures.
🏁 Script executed:
#!/bin/bash
# Check entrypoint script for error handling patterns
cat test/antithesis/image/entrypoint.sh | grep -E 'set -e|exit [1-9]||| exit [1-9]'Length of output: 316
Implement image version tagging and enhance entrypoint error handling
To improve traceability and robustness, please make the following updates:
• In test/antithesis/image/Earthfile
– Replace the hard‑coded ledger:latest tag with a parameterized/versioned tag (e.g. from a Git tag or commit SHA).
– Example diff:
ARG --required ANTITHESIS_REPOSITORY
- SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:latest"
+ ARG IMAGE_TAG?=$(git describe --tags --always)
+ SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:${IMAGE_TAG}"• In test/antithesis/image/entrypoint.sh
– Add strict error handling so failures inside the script halt the container startup.
– Example diff at top of the file:
#!/bin/sh
+ set -euo pipefail
+ trap 'echo "⛔ Entrypoint failed on line $LINENO"; exit 1' ERR
# make sure pg is ready to accept connections
until pg_isready -d ledger -h 10.0.29.16 -U ledger; do
echo "Waiting for postgres at: $POSTGRES_URI"– Ensure any commands (including ledger serve) propagate non‑zero exit codes.
These changes will give you deterministic image tags and guarantee the container fails fast on errors.
📝 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.
| build: | |
| FROM --platform=linux/amd64 ubuntu:latest | |
| RUN apt-get update -y && apt-get install -y postgresql-client curl | |
| COPY (+compile/ledger) /bin/ledger | |
| COPY (+compile/symbols) /symbols | |
| COPY entrypoint.sh /bin/entrypoint.sh | |
| RUN chmod 777 /bin/entrypoint.sh | |
| ENTRYPOINT ["/bin/entrypoint.sh"] | |
| EXPOSE 8080 | |
| ARG --required ANTITHESIS_REPOSITORY | |
| SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:latest" | |
| # test/antithesis/image/Earthfile | |
| build: | |
| FROM --platform=linux/amd64 ubuntu:latest | |
| RUN apt-get update -y && apt-get install -y postgresql-client curl | |
| COPY (+compile/ledger) /bin/ledger | |
| COPY (+compile/symbols) /symbols | |
| COPY entrypoint.sh /bin/entrypoint.sh | |
| RUN chmod 777 /bin/entrypoint.sh | |
| ENTRYPOINT ["/bin/entrypoint.sh"] | |
| EXPOSE 8080 | |
| ARG --required ANTITHESIS_REPOSITORY | |
| ARG IMAGE_TAG?=$(git describe --tags --always) | |
| SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:${IMAGE_TAG}" |
| build: | |
| FROM --platform=linux/amd64 ubuntu:latest | |
| RUN apt-get update -y && apt-get install -y postgresql-client curl | |
| COPY (+compile/ledger) /bin/ledger | |
| COPY (+compile/symbols) /symbols | |
| COPY entrypoint.sh /bin/entrypoint.sh | |
| RUN chmod 777 /bin/entrypoint.sh | |
| ENTRYPOINT ["/bin/entrypoint.sh"] | |
| EXPOSE 8080 | |
| ARG --required ANTITHESIS_REPOSITORY | |
| SAVE IMAGE --push --no-manifest-list "${ANTITHESIS_REPOSITORY}/ledger:latest" | |
| # test/antithesis/image/entrypoint.sh | |
| #!/bin/sh | |
| set -euo pipefail | |
| trap 'echo "⛔ Entrypoint failed on line $LINENO"; exit 1' ERR | |
| # make sure pg is ready to accept connections | |
| until pg_isready -d ledger -h 10.0.29.16 -U ledger; do | |
| echo "Waiting for postgres at: $POSTGRES_URI" | |
| done | |
| # ... rest of your startup logic (e.g. ledger serve) |
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
| if err := systemStore.CreateLedger(ctx, l); err != nil { | ||
| // Handle the case where the ledger already exists |
There was a problem hiding this comment.
I know comments are useful, but this kind of comment just rephrase the code ^^
| @@ -0,0 +1,8 @@ | |||
| VERSION 0.7 | |||
There was a problem hiding this comment.
Stop using Earthfile, there is no Earthfile anymore in the repsoitory ;)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go (2)
12-28: The ledger creation appears correct, but consider adding confirmation logging.The code properly creates a ledger with a randomized name, but it would be helpful to add a success log message to confirm when the ledger has been created successfully. This would make troubleshooting easier when reviewing test logs.
if err != nil { assert.Always(err == nil, "ledger should have been created properly", internal.Details{ "error": err, }) return } + // Print confirmation that ledger was created successfully + fmt.Printf("Ledger '%s' created successfully\n", ledger) }
15-16: Consider checking for overflow when converting to int64.Using the absolute value of a randomly generated big integer and directly converting to int64 could potentially lead to overflow issues if the value exceeds the maximum int64 value.
- id := big.NewInt(0).Abs(internal.RandomBigInt()).Int64() + // Ensure the ID fits within int64 bounds + bigInt := big.NewInt(0).Abs(internal.RandomBigInt()) + // Use Mod to ensure it's within int64 range + maxInt64 := big.NewInt(math.MaxInt64) + id := big.NewInt(0).Mod(bigInt, maxInt64).Int64() ledger := fmt.Sprintf("ledger-%d", id)test/antithesis/workload/bin/cmds/first_default_ledger/main.go (1)
10-25: Consider adding confirmation logging.The code properly creates the default ledger, but lacks any success message that would be helpful for logging and debugging purposes.
if err != nil { assert.Always(err == nil, "ledger should have been created properly", internal.Details{ "error": err, }) return } + fmt.Println("Default ledger created successfully") }Don't forget to add the fmt import if you implement this change.
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
14-24: Add error handling for the edge case of no ledgers.The program should handle the case where no ledgers are returned, which is possible especially during initial setup.
ledgers, err := client.Ledger.V2.ListLedgers(ctx, operations.V2ListLedgersRequest{}) if err != nil { assert.Always(err == nil, "error listing ledgers", internal.Details{ "error": err, }) return } + // Handle the case of no ledgers found + if len(ledgers.V2LedgerListResponse.Cursor.Data) == 0 { + fmt.Println("No ledgers found to check volumes") + return + }test/antithesis/workload/internal/utils.go (3)
22-27: Add more context and validation to RandomBigInt function.The function relies on an external random number generator without validation or documentation about what it returns. Consider adding more context to make the function more robust.
func RandomBigInt() *big.Int { + // random.GetRandom() returns a 64-bit random integer v := random.GetRandom() ret := big.NewInt(0) ret.SetString(fmt.Sprintf("%d", v), 10) + // Ensure the returned value is non-negative as a safeguard + if ret.Sign() < 0 { + ret.Neg(ret) + } return ret }
81-96: Seed the random generator for better randomization.The
rand.Intn()function is used without seeding the random generator, which can lead to predictable sequences if called in quick succession. Consider seeding the random generator for better randomization.func GetRandomLedger(ctx context.Context, client *client.Formance) (string, error) { ledgers, err := ListLedgers(ctx, client) if err != nil { return "", err } if len(ledgers) == 0 { return "", fmt.Errorf("no ledgers found") } // Get a random index within the ledgers slice + // Seed the random generator with current time to ensure randomness + rand.Seed(time.Now().UnixNano()) randomIndex := rand.Intn(len(ledgers)) // Return the ledger at the random index return ledgers[randomIndex], nil }Note: If you're using Go 1.20+, you should use the newer
rand.New(rand.NewSource(time.Now().UnixNano()))approach instead, asrand.Seedis deprecated.
98-114: Hardcoded asset type in RunTx function.The function hardcodes "USD/2" as the asset, which may not be flexible enough for all test scenarios. Consider making the asset type configurable or adding documentation explaining why this specific asset is used.
-func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string) error { +// RunTx creates a transaction on the specified ledger with the given amount of the default asset (USD/2) +// If you need to use a different asset, use the RunTxWithAsset function instead +func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string) error { + return RunTxWithAsset(ctx, client, amount, "USD/2", ledger) +} + +// RunTxWithAsset creates a transaction on the specified ledger with the given amount and asset +func RunTxWithAsset(ctx context.Context, client *client.Formance, amount *big.Int, asset string, ledger string) error { orderID := fmt.Sprint(int64(math.Abs(float64(random.GetRandom())))) dest := fmt.Sprintf("orders:%s", orderID) _, err := client.Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ V2PostTransaction: components.V2PostTransaction{ Postings: []components.V2Posting{{ Amount: amount, - Asset: "USD/2", + Asset: asset, Destination: dest, Source: "world", }}, }, Ledger: ledger, }) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/antithesis.ymlis excluded by!**/*.ymltest/antithesis/workload/go.modis excluded by!**/*.modtest/antithesis/workload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
test/antithesis/workload/bin/cmds/first_default_ledger/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)tools/generator/cmd/root.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/generator/cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/antithesis/workload/bin/cmds/first_default_ledger/main.go (1)
test/antithesis/workload/internal/utils.go (3)
NewClient(41-57)CreateLedger(59-65)Details(20-20)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (4)
test/antithesis/workload/internal/utils.go (2)
NewClient(41-57)ListLedgers(67-79)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)
test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go (1)
test/antithesis/workload/internal/utils.go (4)
NewClient(41-57)RandomBigInt(22-27)CreateLedger(59-65)Details(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests
- GitHub Check: Dirty
- GitHub Check: Push Images to Antithesis
| for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { | ||
| assert.Always( | ||
| volumes.Cmp(new(big.Int)) == 0, | ||
| fmt.Sprintf("aggregated volumes for asset %s should be 0", | ||
| asset, | ||
| ), internal.Details{ | ||
| "error": err, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix incorrect error variable in assertion details.
The err variable in the assertion details is from the GetBalancesAggregated call, which would be nil at this point. The details should contain relevant information about the volumes instead.
for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data {
assert.Always(
volumes.Cmp(new(big.Int)) == 0,
fmt.Sprintf("aggregated volumes for asset %s should be 0",
asset,
), internal.Details{
- "error": err,
+ "asset": asset,
+ "volume": volumes.String(),
})
}🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go around lines 42 to 50, the assertion details incorrectly include the variable 'err', which is nil here. Replace 'err' with relevant information about the 'volumes' variable to provide meaningful context in the assertion failure details.
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (2)
20-30: Error handling could be more descriptive.The error handling is functional but could benefit from a more specific error message that includes the ledger name.
if err != nil { - assert.Always(err == nil, "ledger should have been created properly", internal.Details{ + assert.Always(err == nil, fmt.Sprintf("Failed to create ledger %s", ledger), internal.Details{ "error": err, + "ledger": ledger, }) return }
32-33: Consider adding more detailed logging.The completion message could include the ledger name to make logs more traceable in a parallel environment.
- log.Println("composer: parallel_driver_ledger_create: done") + log.Printf("composer: parallel_driver_ledger_create: done (created ledger: %s)", ledger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
test/antithesis/Earthfile(1 hunks)test/antithesis/workload/Earthfile(1 hunks)test/antithesis/workload/bin/cmds/first_default_ledger/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
- test/antithesis/workload/bin/cmds/first_default_ledger/main.go
- test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go
- test/antithesis/Earthfile
- test/antithesis/workload/Earthfile
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (1)
test/antithesis/workload/internal/utils.go (4)
NewClient(41-57)RandomBigInt(22-27)CreateLedger(59-65)Details(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (3)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (3)
1-12: Good package structure and imports.The package structure and imports are well-organized. The code correctly imports the necessary libraries for context, formatting, logging, and big integer operations, as well as the Antithesis assertion library and internal utilities.
13-19: Solid initialization with unique ledger naming.The initialization logic is sound:
- Properly sets up a background context
- Initializes a client using the internal utility
- Generates a unique ledger name using a random positive ID
Good use of the
Abs()method to ensure the randomly generated ID is positive before formatting it into a ledger name.
1-34:❓ Verification inconclusive
Add cleanup or documentation for created resources.
The program creates a ledger but doesn't have a mechanism to clean it up. While this might be handled elsewhere, it would be beneficial to either:
- Add a cleanup step, or
- Add documentation explaining why cleanup isn't needed or how it's handled
This helps prevent resource leakage in testing environments.
🏁 Script executed:
#!/bin/bash # Check if there are any existing cleanup mechanisms for ledgers in the codebase echo "Searching for ledger cleanup mechanisms..." rg -i "delete\s+ledger|cleanup\s+ledger|remove\s+ledger" --type goLength of output: 313
Add cleanup or document resource handling
I searched the repo for ledger‐cleanup routines and found only:
- internal/storage/system/migrations.go (“Remove ledger state column”)
- pkg/client/v2.go (
DeleteLedgerMetadata) – only deletes metadata, not the ledger itselfThere’s currently no API call in this test CLI to delete the created ledger. To avoid resource leakage in your test environment, please either:
- Invoke or implement a proper
DeleteLedger(or similar) helper after creation, or- Add documentation explaining why ledger cleanup isn’t needed here (e.g. handled by a global teardown or another process)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
53-61: Fix incorrect error variable in assertion details.The
errvariable in the assertion details is from the GetBalancesAggregated call, which would be nil at this point. The details should contain relevant information about the volumes instead.for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { assert.Always( volumes.Cmp(new(big.Int)) == 0, fmt.Sprintf("aggregated volumes for asset %s should be 0", asset, ), internal.Details{ - "error": err, + "asset": asset, + "volume": volumes.String(), }) }
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (2)
39-51: Logging improvement suggestion.Consider adding more detailed logging before making the API call to help with debugging and monitoring the workflow.
func checkVolumes(ctx context.Context, client *client.Formance, ledger string) { + log.Printf("composer: parallel_driver_volumes: checking volumes for ledger %s", ledger) aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ Ledger: ledger, })
17-22: Consider adding a timeout to the context.For better resource management, consider using a context with timeout to prevent indefinite hanging if an operation gets stuck.
func main() { log.Println("composer: parallel_driver_volumes") - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() client := internal.NewClient()Don't forget to add the time import:
import ( "context" "fmt" "log" "math/big" "os" "sync" + "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (4)
test/antithesis/workload/internal/utils.go (3)
NewClient(41-57)ListLedgers(67-79)Details(20-20)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)
🔇 Additional comments (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
17-37: LGTM: Good concurrency implementation with WaitGroup.The main function correctly implements the WaitGroup pattern for concurrency, ensuring all goroutines complete before the program exits. This addresses the synchronization issue that was mentioned in past reviews.
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
17-38: Consider adding a limit to concurrent goroutines.While the WaitGroup implementation is correct, launching a goroutine for each ledger without limits could lead to resource exhaustion if there are many ledgers. Consider implementing a worker pool pattern or semaphore to limit concurrency.
- wg := sync.WaitGroup{} - for _, ledger := range ledgers.V2LedgerListResponse.Cursor.Data { - wg.Add(1) - go func(ledger string) { - defer wg.Done() - checkVolumes(ctx, client, ledger) - }(ledger.Name) - } - wg.Wait() + // Limit concurrency to avoid resource exhaustion + const maxConcurrent = 10 + sem := make(chan struct{}, maxConcurrent) + wg := sync.WaitGroup{} + + for _, ledger := range ledgers.V2LedgerListResponse.Cursor.Data { + wg.Add(1) + sem <- struct{}{} // Acquire semaphore + go func(ledger string) { + defer wg.Done() + defer func() { <-sem }() // Release semaphore + checkVolumes(ctx, client, ledger) + }(ledger.Name) + } + wg.Wait()
29-37: Consider adding timeout to the context.The current implementation uses a background context without a timeout, which could lead to hanging goroutines if the ledger service is unresponsive. Adding a timeout would ensure the program doesn't hang indefinitely.
- ctx := context.Background() + // Add timeout to prevent hanging if service is unresponsive + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel()Don't forget to add the time package to your imports.
48-56: Potential issue with error reference in assertion details.The
errvariable in the details is from the context of getting aggregated balances, which has already been checked for nil. If the assertion reaches this point,errwill always be nil, making its inclusion in the details redundant.- ), internal.Details{ - "error": err, - }) + ), internal.Details{ + "ledger": ledger, + "asset": asset, + })
49-51: Make volume comparison more explicit.While
volumes.Cmp(new(big.Int)) == 0correctly checks if volumes equals zero, it could be more readable with a comment or by using a dedicated function/variable for clarity.- volumes.Cmp(new(big.Int)) == 0, + // Check if volumes equals zero + volumes.Cmp(big.NewInt(0)) == 0,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)
🔇 Additional comments (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (1)
3-15: Import organization looks good.The imports are well organized with standard library packages grouped together, followed by external dependencies. All imports appear to be used appropriately throughout the code.
| func checkVolumes(ctx context.Context, client *client.Formance, ledger string) { | ||
| aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ | ||
| Ledger: ledger, | ||
| }) | ||
| if err != nil { | ||
| log.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling terminates all goroutines.
Using log.Fatalf in a goroutine will terminate the entire program, not just the current goroutine. This means if one ledger check fails, the others won't be completed.
- if err != nil {
- log.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err)
- }
+ if err != nil {
+ log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err)
+ return
+ }📝 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.
| func checkVolumes(ctx context.Context, client *client.Formance, ledger string) { | |
| aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ | |
| Ledger: ledger, | |
| }) | |
| if err != nil { | |
| log.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err) | |
| } | |
| func checkVolumes(ctx context.Context, client *client.Formance, ledger string) { | |
| aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ | |
| Ledger: ledger, | |
| }) | |
| if err != nil { | |
| log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err) | |
| return | |
| } | |
| // ... rest of function ... | |
| } |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/eventually_correct/main.go around lines 40 to 47, replace the use of log.Fatalf with error handling that does not terminate the entire program. Instead of calling log.Fatalf on error, return the error or send it through a channel so the caller can handle it appropriately, allowing other goroutines to continue running and complete their ledger checks.
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (1)
45-57:⚠️ Potential issueFix error handling to avoid terminating all goroutines.
Using
log.Fatalfin a goroutine will terminate the entire program, not just the current goroutine. This means if one ledger check fails, the others won't be completed.if err != nil { if internal.IsServerError(aggregated.GetHTTPMeta()) { assert.Always( false, fmt.Sprintf("error getting aggregated balances for ledger %s: %s", ledger, err), internal.Details{ "error": err, }, ) } else { - log.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err) + log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err) + return } }
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (2)
18-39: Consider limiting concurrency for large numbers of ledgers.The current implementation creates a goroutine for each ledger, which could potentially launch too many concurrent HTTP requests if there are many ledgers. Consider implementing a worker pool pattern to limit concurrency.
Here's a potential implementation using a worker pool:
func main() { log.Println("composer: eventually_correct") ctx := context.Background() client := internal.NewClient() ledgers, err := client.Ledger.V2.ListLedgers(ctx, operations.V2ListLedgersRequest{}) if err != nil { log.Printf("error listing ledgers: %s", err) os.Exit(1) } + // Limit concurrency to a reasonable number + maxConcurrency := 10 + semaphore := make(chan struct{}, maxConcurrency) wg := sync.WaitGroup{} for _, ledger := range ledgers.V2LedgerListResponse.Cursor.Data { wg.Add(1) + // Acquire a slot from the semaphore + semaphore <- struct{}{} go func(ledger string) { defer wg.Done() + defer func() { + // Release the slot back to the semaphore + <-semaphore + }() checkVolumes(ctx, client, ledger) }(ledger.Name) } wg.Wait() }
41-42: Add function documentation.Add documentation comments to describe the purpose of the function and its parameters to improve code maintainability.
+// checkVolumes fetches aggregated balances for the specified ledger and asserts +// that all asset volumes are zero. It uses Antithesis assert.Always to record +// any violations while allowing the program to continue checking other assets. func checkVolumes(ctx context.Context, client *client.Formance, ledger string) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/storage/driver/driver.go(4 hunks)test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/antithesis/workload/internal/utils.go
- internal/storage/driver/driver.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
test/antithesis/workload/internal/utils.go (3)
NewClient(41-57)ListLedgers(74-86)IsServerError(59-61)pkg/client/v2.go (1)
V2(19-21)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)pkg/client/formance.go (1)
Formance(70-74)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
| func IsServerError(httpMeta components.HTTPMetadata) bool { | ||
| return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate IsServerError function.
This function duplicates the functionality already available in internal.IsServerError which is correctly used at line 46. Remove this duplicate implementation to avoid potential inconsistencies.
-func IsServerError(httpMeta components.HTTPMetadata) bool {
- return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600
-}📝 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.
| func IsServerError(httpMeta components.HTTPMetadata) bool { | |
| return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 | |
| } |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/eventually_correct/main.go around lines 72 to 74, there is a duplicate implementation of the IsServerError function which duplicates the existing internal.IsServerError function used at line 46. Remove this duplicate IsServerError function entirely to avoid inconsistencies and rely on the internal.IsServerError function instead.
| "error": err, | ||
| }) |
There was a problem hiding this comment.
Incorrect error variable in assert details.
The err variable may be nil at this point since this code executes after the error check. Include the actual volume value instead to provide more meaningful assertion details.
assert.Always(
volumes.Cmp(new(big.Int)) == 0,
fmt.Sprintf("aggregated volumes for asset %s should be 0",
asset,
), internal.Details{
- "error": err,
+ "actual_volume": volumes.String(),
+ "asset": asset,
})📝 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.
| "error": err, | |
| }) | |
| assert.Always( | |
| volumes.Cmp(new(big.Int)) == 0, | |
| fmt.Sprintf("aggregated volumes for asset %s should be 0", | |
| asset, | |
| ), internal.Details{ | |
| "actual_volume": volumes.String(), | |
| "asset": asset, | |
| }) |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/eventually_correct/main.go at lines 65-66, the assertion details incorrectly include the variable 'err', which may be nil since this code runs after the error check. Replace 'err' with the actual volume value in the assertion details to provide more meaningful and accurate information.
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (1)
5-8: Consider enhancing the placeholder with more context about the planned functionality.This is currently just a placeholder implementation, which is fine for initial setup. However, consider adding more detailed comments about the intended functionality of this command, particularly around how it will handle version upgrades within the Antithesis testing framework.
According to the PR context, this appears to be part of a suite of workload orchestration tools. It would be helpful to document:
- The purpose of this specific command
- How it will interact with other components
- Expected inputs/outputs when fully implemented
func main() { - // TODO: implement + // TODO: implement version upgrade functionality + // This command will handle dynamic version upgrades during Antithesis testing. + // It should: + // - Coordinate with the test orchestrator + // - Manage the upgrade process for specific components + // - Validate system stability after upgrades log.Println("placeholder command for anytime_version_upgrade") }test/antithesis/workload/datagen/main.go (2)
1-7: Good starting point for the package, but consider improving imports.The imports look good, but note that
math/randis deprecated in favor ofmath/rand/v2in Go 1.23+. Consider using the newer version for improved randomness and future compatibility.import ( "fmt" - "math/rand" + "math/rand/v2" )
15-17: Add documentation to explain the program's purpose.The main function works correctly but lacks documentation explaining the program's purpose within the testing framework.
+// main generates a random account number and prints it to stdout +// This is used for data generation in Antithesis testing workloads func main() { fmt.Println(Account()) }test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (2)
72-83: Transaction function always uses the same fixed values.The
Transaction()function always returns a posting with the same fixed values, which may not thoroughly test the system under different scenarios.func Transaction() []components.V2Posting { postings := []components.V2Posting{} postings = append(postings, components.V2Posting{ - Amount: big.NewInt(100), + Amount: internal.RandomBigInt(), // Use random amounts for better coverage Asset: "USD/2", - Destination: "orders:1234", + Destination: fmt.Sprintf("orders:%d", rand.Intn(10000)), // Randomize order IDs Source: "world", }) return postings }
85-93: Unused function in the main flow.The
Sequence()function is defined but never used in the program. Consider either removing it or documenting why it's defined for future use.-func Sequence() []Postings { - postings := []Postings{} - - for i := 0; i < 10; i++ { - postings = append(postings, Transaction()) - } - - return postings -}If you need to keep this function for future use, add appropriate documentation:
+// Sequence returns a sequence of 10 transactions +// Note: Currently not used in the main flow but kept for future expansion func Sequence() []Postings { postings := []Postings{} for i := 0; i < 10; i++ { postings = append(postings, Transaction()) } return postings }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/antithesis/workload/datagen/example.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go(1 hunks)test/antithesis/workload/bin/cmds/first_default_ledger/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/datagen/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/antithesis/workload/bin/cmds/first_default_ledger/main.go
- test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go
- test/antithesis/workload/internal/utils.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (2)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (1)
1-4: LGTM: Package declaration and imports look good.The package declaration and import statement follow Go conventions. Using the standard library's "log" package is appropriate for basic logging functionality.
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (1)
3-14: LGTM: Well-organized imports with clear separation.The import organization is good, with standard library packages first, followed by third-party packages.
| type Sequence struct { | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or properly implement the unused struct.
The Sequence struct is defined but never used in this file. Either remove it or document its purpose and implement its intended functionality.
-type Sequence struct {
-}📝 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.
| type Sequence struct { | |
| } |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/datagen/main.go at lines 8 to 9, the Sequence struct
is defined but not used or implemented. Either remove the Sequence struct
entirely if it is unnecessary, or provide a proper implementation and
documentation explaining its purpose and how it fits into the codebase.
| func Account() string { | ||
| return fmt.Sprintf("%d", rand.Intn(10e9)) | ||
| } |
There was a problem hiding this comment.
Account generation is not seeded, which will produce predictable results.
The random number generator is not seeded, which means it will produce the same sequence of "random" numbers on each program run. This could lead to predictable account numbers, which may be problematic for testing.
+func init() {
+ // Seed the random number generator to ensure different account numbers on each run
+ rand.Seed(time.Now().UnixNano())
+}
func Account() string {
return fmt.Sprintf("%d", rand.Intn(10e9))
}You'll also need to add "time" to your imports.
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/datagen/main.go around lines 11 to 13, the Account
function uses rand.Intn without seeding the random number generator, causing
predictable outputs. To fix this, seed the random number generator once at
program startup using rand.Seed with the current time in nanoseconds from
time.Now().UnixNano(). Also, add "time" to the import statements to enable this.
This ensures Account generates different values on each run.
| res, err := RunTx(ctx, client, Transaction(), ledger) | ||
| assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{ | ||
| "ledger": ledger, | ||
| }) | ||
| assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{ | ||
| "ledger": ledger, | ||
| "error": err, | ||
| }) |
There was a problem hiding this comment.
Improve error handling in transaction assertions.
The code uses assertions but doesn't properly handle the case where res could be nil if err is not nil.
res, err := RunTx(ctx, client, Transaction(), ledger)
assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{
"ledger": ledger,
})
- assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{
+ assert.Always(err == nil || !internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{
"ledger": ledger,
"error": err,
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 38 to 45
57E6
, the code asserts conditions on res without checking if res is
nil when err is not nil, which can cause a nil pointer dereference. Add a check
to ensure res is not nil before calling res.GetHTTPMeta(), and handle the case
where err is not nil by skipping or adjusting the assertions accordingly to
avoid runtime panics.
| totalAmount := big.NewInt(0) | ||
|
|
||
| pool := pond.New(10, 10e3) | ||
|
|
||
| for i := 0; i < count; i++ { | ||
| amount := internal.RandomBigInt() | ||
| totalAmount = totalAmount.Add(totalAmount, amount) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused variable tracking.
The totalAmount variable is incremented with random values that are never used in transactions. The amount generated in each loop iteration is also never used for the transaction since Transaction() uses a fixed amount of 100.
- totalAmount := big.NewInt(0)
-
pool := pond.New(10, 10e3)
for i := 0; i < count; i++ {
- amount := internal.RandomBigInt()
- totalAmount = totalAmount.Add(totalAmount, amount)
pool.Submit(func() {📝 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.
| totalAmount := big.NewInt(0) | |
| pool := pond.New(10, 10e3) | |
| for i := 0; i < count; i++ { | |
| amount := internal.RandomBigInt() | |
| totalAmount = totalAmount.Add(totalAmount, amount) | |
| pool := pond.New(10, 10e3) | |
| for i := 0; i < count; i++ { | |
| pool.Submit(func() { | |
| // ... | |
| }) | |
| } |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 30 to 36, the variable totalAmount is updated with random amounts that are
never used, and the generated amount is not passed to the Transaction function
which uses a fixed value of 100. To fix this, remove the totalAmount variable
and the addition logic since it is unused, and modify the Transaction calls to
use the generated amount instead of the fixed 100 value so that the random
amounts are actually used in the transactions.
| pool.Submit(func() { | ||
| res, err := RunTx(ctx, client, Transaction(), ledger) | ||
| assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{ | ||
| "ledger": ledger, | ||
| }) | ||
| assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{ | ||
| "ledger": ledger, | ||
| "error": err, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Potential race condition in anonymous function.
The anonymous function is accessing variables from the outer scope (ctx, client, ledger), which could lead to race conditions, especially if these variables change during execution.
pool.Submit(func() {
- res, err := RunTx(ctx, client, Transaction(), ledger)
+ // Capture values to prevent race conditions
+ localCtx := ctx
+ localClient := client
+ localLedger := ledger
+ res, err := RunTx(localCtx, localClient, Transaction(), localLedger)
assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{
- "ledger": ledger,
+ "ledger": localLedger,
})
assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{
- "ledger": ledger,
+ "ledger": localLedger,
"error": err,
})
})📝 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.
| pool.Submit(func() { | |
| res, err := RunTx(ctx, client, Transaction(), ledger) | |
| assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{ | |
| "ledger": ledger, | |
| }) | |
| assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{ | |
| "ledger": ledger, | |
| "error": err, | |
| }) | |
| }) | |
| pool.Submit(func() { | |
| // Capture values to prevent race conditions | |
| localCtx := ctx | |
| localClient := client | |
| localLedger := ledger | |
| res, err := RunTx(localCtx, localClient, Transaction(), localLedger) | |
| assert.Sometimes(err == nil, "transaction was committed successfully", internal.Details{ | |
| "ledger": localLedger, | |
| }) | |
| assert.Always(!internal.IsServerError(res.GetHTTPMeta()), "no internal server error when committing transaction", internal.Details{ | |
| "ledger": localLedger, | |
| "error": err, | |
| }) | |
| }) |
🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 37 to 46, the anonymous function passed to pool.Submit captures outer
variables ctx, client, and ledger directly, which may cause race conditions if
these variables change concurrently. To fix this, pass these variables as
parameters to the anonymous function or create local copies inside the loop
before submitting the function, ensuring each goroutine uses its own independent
copy of the variables.
No description provided.