feat: add DST setup #879
feat: add DST setup #879
Conversation
Signed-off-by: Clément Salaün <[email protected]>
Signed-off-by: Clément Salaün <[email protected]>
WalkthroughCreateLedger was moved to run entirely inside a database transaction with explicit constraint mapping, bucket retrieval, init/migration, up-to-date checks, and ledger-store creation. Numerous Antithesis test workloads, helper utilities, Dockerfiles, entrypoint script, and Justfile recipes were added for parallel workloads, ledger operations, health checks, and experiment orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Driver as Driver.CreateLedger
participant DB as Database
participant Bucket as BucketService
participant Store as LedgerStoreFactory
Caller->>Driver: CreateLedger(ctx, ledger)
Driver->>DB: RunInTx(BEGIN)
Note over Driver,DB: Insert system ledger
DB-->>Driver: ok / constraint error
alt constraint violation
Driver->>DB: ROLLBACK
Driver-->>Caller: ErrLedgerAlreadyExists
else success
Driver->>Bucket: GetBucket(ledger)
Bucket-->>Driver: bucket, initialized?
alt bucket initialized
Driver->>Bucket: CheckUpToDate
alt outdated
Driver->>DB: ROLLBACK
Driver-->>Caller: ErrBucketOutdated
else up-to-date
Driver->>Bucket: AddLedgerToBucket
end
else not initialized
Driver->>Bucket: MigrateBucketSchema
end
Driver->>Store: NewStore(bucket, ledger)
Store-->>Driver: store
Driver->>DB: COMMIT
Driver-->>Caller: store, nil
end
sequenceDiagram
participant Init as workload/bin/init
participant LedgerAPI as Ledger service
participant Workload as Workload binaries
Init->>LedgerAPI: GET /ledgers (loop)
alt not ready
LedgerAPI-->>Init: error -> retry
else ready
LedgerAPI-->>Init: ok
Init->>Workload: start workloads (parallel)
par
Workload->>LedgerAPI: CreateLedger / CreateTransaction / Balance checks
and
Workload->>LedgerAPI: Version upgrade / other ops
end
Workload-->>Init: done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #879 +/- ##
==========================================
+ Coverage 80.67% 80.72% +0.04%
==========================================
Files 186 186
Lines 10309 10309
==========================================
+ Hits 8317 8322 +5
+ Misses 1569 1565 -4
+ Partials 423 422 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/bin/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… | |
| } |
test/antithesis/image/entrypoint.sh
Outdated
| 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
}
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.
test/antithesis/workload/Earthfile
Outdated
| 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/[email protected] # 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 |
internal/storage/driver/driver.go
Outdated
| 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
test/antithesis/image/Earthfile
Outdated
| 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" No newline at end of file |
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 <[email protected]>
Signed-off-by: Clément Salaün <[email protected]>
Signed-off-by: Clément Salaün <[email protected]>
internal/storage/driver/driver.go
Outdated
|
|
||
| // Step 1: Create the ledger record in the system store | ||
| 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 ^^
pkg/client/Earthfile
Outdated
| @@ -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 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
81-83: Clarify Sometimes-assert messageThe predicate asserts that errors may occur; adjust message to match intent.
- assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ + assert.Sometimes(err != nil, "GetAccount may transiently fail", internal.Details{ "error": err, })
132-134: Remove panic; rely on assertion for failure reportingPanicking in a goroutine kills the whole workload; the assert below already captures the invariant.
- if tx.ID.Cmp(expectedTxId) != 0 { - panic(fmt.Sprintf("oop: expected %v and got %v", expectedTxId, tx.ID)) - }
121-121: Use log.Printf and include newlineMinor logging consistency.
- fmt.Printf("error listing transactions: %v", err) + log.Printf("error listing transactions: %v", err)
13-13: Drop unused import introduced by the duplicate IsServerErrorOnce the helper is removed, this import is unused.
- "github.com/formancehq/ledger/pkg/client/models/components"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.github/workflows/antithesis_daily.ymlis excluded by!**/*.yml.github/workflows/antithesis_release.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go(1 hunks)test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
- test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
test/antithesis/workload/internal/utils.go (4)
NewClient(43-63)ListLedgers(80-92)Details(22-22)USER_ACCOUNT_COUNT(20-20)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-24)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(37-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (5)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (5)
61-69: Fix assertion details: err is nil here; include actual volumeUse the actual value to make the failure actionable.
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, + "actual_volume": volumes.String(), }) }
112-115: LGTM on paginationPassing the cursor and advancing it removes the infinite-loop risk. Good fix.
Also applies to: 140-144
148-150: Remove duplicate IsServerError; use internal.IsServerError everywhereThis duplicates internal.IsServerError and risks divergence.
-func IsServerError(httpMeta components.HTTPMetadata) bool { - return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 -}Note: also remove the now-unused import of components (see next comment).
47-59: Avoid nil deref, don’t Fatalf in goroutines, and return after handling error
- aggregated may be nil; guard before GetHTTPMeta().
- log.Fatalf inside these goroutines kills the whole program; prefer non-fatal log + return.
- Ensure early-return on error to avoid using aggregated later.
- if err != nil { - if internal.IsServerError(aggregated.GetHTTPMeta()) { + if err != nil { + if aggregated != nil && 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 }
75-76: Ensure Go toolchain ≥ 1.22 or replace range-over-integerGo 1.22 introduced "for i := range N". This repo has no go.mod or CI Go-version and the code at test/antithesis/workload/bin/cmds/eventually_correct/main.go:75–76 uses
for i := range internal.USER_ACCOUNT_COUNT; either ensure builds use Go ≥1.22 or change to a classic loop (USER_ACCOUNT_COUNT is uint64):for i := uint64(0); i < internal.USER_ACCOUNT_COUNT; i++ {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (3)
81-83: Clarify Sometimes assertion semanticsMessage says “Client can aggregate…”, but the condition asserts an error. Likely should assert success.
Apply this diff (if intent is to assert occasional success):
- assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ + assert.Sometimes(err == nil, "Client can aggregate account balances", internal.Details{ "error": err, })Please confirm intended semantics of Sometimes in this suite.
126-129: Avoid mutating the transaction’s ID via shared big.Int pointerCopy the value before arithmetic;
Add/Submutate the receiver.Apply this diff:
- if expectedTxId == nil { - expectedTxId = transactions.V2TransactionsCursorResponse.Cursor.Data[0].ID - expectedTxId.Add(expectedTxId, big.NewInt(1)) - } + if expectedTxId == nil { + // copy value to avoid mutating the transaction ID + expectedTxId = new(big.Int).Set(transactions.V2TransactionsCursorResponse.Cursor.Data[0].ID) + expectedTxId.Add(expectedTxId, big.NewInt(1)) + }
120-121: Use consistent loggingPrefer
log.Printffor consistency with the rest of the file.Apply this diff:
- fmt.Printf("error listing transactions: %v", err) + log.Printf("error listing transactions: %v", err)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)
🚧 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
🧠 Learnings (1)
📚 Learning: 2025-05-20T13:08:10.034Z
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
🧬 Code graph analysis (1)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
test/antithesis/workload/internal/utils.go (3)
NewClient(43-63)Details(22-22)USER_ACCOUNT_COUNT(20-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(37-61)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests
- GitHub Check: Dirty
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (4)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
61-69: Assertion details should include actual values, not a possibly nil errorProvide actionable context by reporting the asset and the actual aggregated volume.
Apply this diff:
- assert.Always( - volumes.Cmp(new(big.Int)) == 0, - fmt.Sprintf("aggregated volumes for asset %s should be 0", - asset, - ), internal.Details{ - "error": err, - }) + assert.Always( + volumes.Cmp(new(big.Int)) == 0, + fmt.Sprintf("aggregated volumes for asset %s should be 0", asset), + internal.Details{ + "asset": asset, + "actual_volume": volumes.String(), + }, + )
108-141: Pagination logic looks goodUsing
Cursor/HasMoreprevents the infinite-loop hazard and iterates pages correctly.
145-147: Remove duplicate IsServerError; use internal.IsServerErrorThis duplicates existing logic used above.
Apply this diff:
-func IsServerError(httpMeta components.HTTPMetadata) bool { - return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 -}
47-59: Avoid nil deref and don’t terminate the program from a goroutine; return after handling errorsGuard
aggregatedbeforeGetHTTPMeta(), avoidlog.Fatalfinside goroutines, and make sure toreturnto prevent panics on nilaggregated.Apply this diff:
- 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) - } - } + if err != nil { + // aggregated may be nil on error + var httpMeta components.HTTPMetadata + if aggregated != nil { + httpMeta = aggregated.GetHTTPMeta() + } + if internal.IsServerError(httpMeta) { + assert.Always(false, + fmt.Sprintf("error getting aggregated balances for ledger %s: %s", ledger, err), + internal.Details{"error": err, "ledger": ledger}, + ) + return + } + log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err) + return + }
| for i := range internal.USER_ACCOUNT_COUNT { | ||
| address := fmt.Sprintf("users:%d", i) | ||
| account, err := client.Ledger.V2.GetAccount(ctx, operations.V2GetAccountRequest{ | ||
| Ledger: ledger, | ||
| Address: address, | ||
| }) |
There was a problem hiding this comment.
Go does not support ranging over an integer
This does not compile. Use a classic indexed loop.
Apply this diff:
- for i := range internal.USER_ACCOUNT_COUNT {
- address := fmt.Sprintf("users:%d", i)
+ for i := 0; i < int(internal.USER_ACCOUNT_COUNT); i++ {
+ address := fmt.Sprintf("users:%d", i)📝 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 i := range internal.USER_ACCOUNT_COUNT { | |
| address := fmt.Sprintf("users:%d", i) | |
| account, err := client.Ledger.V2.GetAccount(ctx, operations.V2GetAccountRequest{ | |
| Ledger: ledger, | |
| Address: address, | |
| }) | |
| for i := 0; i < int(internal.USER_ACCOUNT_COUNT); i++ { | |
| address := fmt.Sprintf("users:%d", i) | |
| account, err := client.Ledger.V2.GetAccount(ctx, operations.V2GetAccountRequest{ | |
| Ledger: ledger, | |
| Address: address, | |
| }) |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/eventually_correct/main.go around lines 75
to 80, the code incorrectly uses "for i := range internal.USER_ACCOUNT_COUNT"
which fails because Go cannot range over an integer; replace it with a classic
indexed loop such as "for i := 0; i < internal.USER_ACCOUNT_COUNT; i++"
(ensuring USER_ACCOUNT_COUNT is an int or cast as needed) and keep the body
unchanged so address := fmt.Sprintf("users:%d", i) and the subsequent GetAccount
call continue to use the loop index.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (1)
48-52: Optional: Avoid name shadowing (clientparam vs imported package).Rename the parameter for readability.
Apply this diff:
-func CreateTransaction( - ctx context.Context, - client *client.Formance, - ledger string, -) { +func CreateTransaction( + ctx context.Context, + cli *client.Formance, + ledger string, +) { @@ - res, err := client.Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ + res, err := cli.Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{test/antithesis/workload/bin/cmds/eventually_correct/main.go (1)
123-123: Use log.Printf for consistencyAlign logging with the rest of the file and include timestamps.
- fmt.Printf("error listing transactions: %v", err) + log.Printf("error listing transactions: %v", err)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go(1 hunks)test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-20T13:08:10.034Z
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
📚 Learning: 2025-02-24T11:31:17.135Z
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:31-34
Timestamp: 2025-02-24T11:31:17.135Z
Learning: In Go, using range with an integer constant (e.g., `for i := range countLedgers`) is valid syntax and compiles correctly.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
🧬 Code graph analysis (3)
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (5)
test/antithesis/workload/internal/utils.go (5)
NewClient(43-63)GetRandomLedger(94-107)Details(22-22)GetRandomAddress(109-111)RandomBigInt(24-29)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumInsufficientFund(14-14)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
test/antithesis/workload/internal/utils.go (5)
NewClient(43-63)ListLedgers(80-92)IsServerError(65-67)Details(22-22)USER_ACCOUNT_COUNT(20-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(37-61)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (1)
test/antithesis/workload/internal/utils.go (1)
Details(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (9)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (3)
3-17: Add missing imports for trimming and timeouts (and in-cluster config if adopted).Required for the fixes above and the timeout/config changes below.
Apply this diff:
import ( "context" "log" "os" + "strings" + "time" @@ "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ + "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" )
26-35: Prefer in-cluster config with local kubeconfig fallback.Works in pods and locally without changing flags.
Apply this diff:
- // build dynamic client - config, err := clientcmd.BuildConfigFromFlags("", "") - if err != nil { - panic(err) - } + // build dynamic client (prefer in-cluster, fallback to local) + config, err := rest.InClusterConfig() + if err != nil { + config, err = clientcmd.BuildConfigFromFlags("", "") + if err != nil { + panic(err) + } + }
43-57: Optional: Use MergePatch to update spec.version directly.Reduces read/modify/write races and payload size; avoids field drift.
Apply this diff if you prefer Patch over Update:
- // fetch the previous Ledger resource - res, err := dyn.Resource(gvr).Get(context.Background(), "stack0-ledger", metav1.GetOptions{}) - if err != nil { - panic(err) - } - - // set the version to the latest tag - unstructured.SetNestedField(res.Object, string(latest_tag), "spec", "version") - - res, err = dyn.Resource(gvr).Update(context.Background(), res, metav1.UpdateOptions{}) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + payload, _ := json.Marshal(map[string]any{ + "spec": map[string]any{"version": tag}, + }) + res, err := dyn.Resource(gvr). + Namespace("formance-systems"). + Patch(ctx, "stack0-ledger", types.MergePatchType, payload, metav1.PatchOptions{})test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (1)
33-39: Optional: Capture outer variables for clarity in goroutines.Not strictly necessary here, but avoids surprises if the variables become mutable later.
Apply this diff:
- for i := 0; i < count; i++ { - pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) - } + for i := 0; i < count; i++ { + lctx, lclient, lledger := ctx, client, ledger + pool.Submit(func() { CreateTransaction(lctx, lclient, lledger) }) + }test/antithesis/workload/bin/cmds/eventually_correct/main.go (5)
61-69: Assertion details: include actual volume; avoid nil err in details
errwill be nil here; include the measured volume (and keep asset) for useful diagnostics.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, - }) + ), internal.Details{ + "asset": asset, + "actual_volume": volumes.String(), + }) }
75-80: LGTM: integer range loop is validUsing
for i := range internal.USER_ACCOUNT_COUNTis valid with recent Go versions and reads clearly.
111-144: LGTM: pagination and sequential ID checksGood use of cursor-based pagination and stable ordering to verify sequential IDs across pages.
148-150: Remove duplicate IsServerError; use internal.IsServerErrorThis duplicates
internal.IsServerError. Remove to avoid drift.-func IsServerError(httpMeta components.HTTPMetadata) bool { - return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 -}Note: If nothing else uses
componentsafter this change, drop that import too; otherwise the import stays needed if you adopt the earlierhttpMetafix.
47-59: Fix nil-deref and avoid Fatalf in goroutine; return after handling error
aggregatedcan be nil whenerr != nilleading to a panic onGetHTTPMeta(). Also,log.Fatalfinside a goroutine terminates the whole program. BuildhttpMetasafely, useinternal.IsServerError, log non-server errors, and return to avoid subsequent nil use.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) - } + httpMeta := components.HTTPMetadata{} + if aggregated != nil { + httpMeta = aggregated.GetHTTPMeta() + } + if internal.IsServerError(httpMeta) { + assert.Always( + false, + fmt.Sprintf("error getting aggregated balances for ledger %s: %s", ledger, err), + internal.Details{ + "error": err, + }, + ) + } else { + log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err) + } + return }
| latest_tag, err := os.ReadFile("/ledger_latest_tag") | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Trim newline from /ledger_latest_tag and validate non-empty; use the trimmed value when setting spec.version.
Using the raw file contents can include a trailing newline and empty values.
Apply this diff:
- latest_tag, err := os.ReadFile("/ledger_latest_tag")
+ b, err := os.ReadFile("/ledger_latest_tag")
if err != nil {
log.Fatal(err)
}
+ tag := strings.TrimSpace(string(b))
+ if tag == "" {
+ log.Fatal("empty LEDGER_LATEST_TAG")
+ }
@@
- // set the version to the latest tag
- unstructured.SetNestedField(res.Object, string(latest_tag), "spec", "version")
+ // set the version to the latest tag
+ if err := unstructured.SetNestedField(res.Object, tag, "spec", "version"); err != nil {
+ log.Fatal(err)
+ }Also applies to: 49-51
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go around
lines 21-24 (and also apply the same change at lines 49-51), replace the raw
os.ReadFile usage with reading the file, trimming whitespace/newlines (e.g.,
strings.TrimSpace) and validating the result is non-empty; if trimmed value is
empty return or log.Fatal with a clear message, and use the trimmed value when
assigning spec.version so you don't propagate trailing newlines or empty values.
| // fetch the previous Ledger resource | ||
| res, err := dyn.Resource(gvr).Get(context.Background(), "stack0-ledger", metav1.GetOptions{}) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Add namespace to requests and use a bounded timeout context.
CR is namespaced; missing Namespace(...) will fail. Also avoid unbounded calls.
Apply this diff:
- // fetch the previous Ledger resource
- res, err := dyn.Resource(gvr).Get(context.Background(), "stack0-ledger", metav1.GetOptions{})
+ // fetch the previous Ledger resource
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ res, err := dyn.Resource(gvr).
+ Namespace("formance-systems").
+ Get(ctx, "stack0-ledger", metav1.GetOptions{})
@@
- res, err = dyn.Resource(gvr).Update(context.Background(), res, metav1.UpdateOptions{})
+ res, err = dyn.Resource(gvr).
+ Namespace("formance-systems").
+ Update(ctx, res, metav1.UpdateOptions{})Also applies to: 52-56
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go around
lines 43-47 (and similarly 52-56), the dynamic client Get calls are missing a
Namespace(...) and use an unbounded context; update both calls to use a bounded
context created via context.WithTimeout (e.g., 5–10s) with defer cancel(), and
chain Namespace("your-namespace") before Resource(gvr).Get(...); ensure you pass
the timeout context into Get and replace any context.Background() used there
with the new ctx.
| ledger, err := internal.GetRandomLedger(ctx, client) | ||
| if err != nil { | ||
| ledger = "default" | ||
| log.Printf("error getting random ledger: %s", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Don’t return after fallback to default ledger.
Currently exits before submitting any work.
Apply this diff:
ledger, err := internal.GetRandomLedger(ctx, client)
if err != nil {
ledger = "default"
log.Printf("error getting random ledger: %s", 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.
| ledger, err := internal.GetRandomLedger(ctx, client) | |
| if err != nil { | |
| ledger = "default" | |
| log.Printf("error getting random ledger: %s", err) | |
| return | |
| } | |
| ledger, err := internal.GetRandomLedger(ctx, client) | |
| if err != nil { | |
| ledger = "default" | |
| log.Printf("error getting random ledger: %s", err) | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 24-29, the code currently returns immediately after falling back to the
default ledger which prevents any work from being submitted; remove the
premature return so that after logging the error and setting ledger = "default"
execution continues and the program proceeds to submit transactions using the
default ledger (ensure no other control flow relies on the returned error).
| const count = 100 | ||
|
|
||
| pool := pond.New(10, 10e3) | ||
|
|
||
| for range count { | ||
| pool.Submit(func() { | ||
| CreateTransaction(ctx, client, ledger) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix invalid loop: for range count won’t compile.
Use a standard counted loop.
Apply this diff:
- const count = 100
+ const count = 100
@@
- for range count {
- pool.Submit(func() {
- CreateTransaction(ctx, client, ledger)
- })
- }
+ for i := 0; i < count; i++ {
+ pool.Submit(func() { CreateTransaction(ctx, client, ledger) })
+ }📝 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.
| const count = 100 | |
| pool := pond.New(10, 10e3) | |
| for range count { | |
| pool.Submit(func() { | |
| CreateTransaction(ctx, client, ledger) | |
| }) | |
| } | |
| const count = 100 | |
| pool := pond.New(10, 10e3) | |
| for i := 0; i < count; i++ { | |
| pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 31 to 39, the loop uses the invalid construct "for range count" which
won't compile; replace it with a standard counted loop such as "for i := 0; i <
count; i++ { pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) }"
so the loop iterates count times and submits a task for each iteration.
| func RandomPostings() []components.V2Posting { | ||
| postings := []components.V2Posting{} | ||
|
|
||
| for range random.GetRandom()%20+1 { | ||
| source := internal.GetRandomAddress() | ||
| destination := internal.GetRandomAddress() | ||
| amount := internal.RandomBigInt() | ||
| asset := random.RandomChoice([]string{"USD/2", "EUR/2", "COIN"}) | ||
|
|
||
| postings = append(postings, components.V2Posting{ | ||
| Amount: amount, | ||
| Asset: asset, | ||
| Destination: destination, | ||
| Source: source, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix invalid loop in RandomPostings and preallocate slice.
for range <int> is invalid Go; also preallocate to reduce allocations.
Apply this diff:
-func RandomPostings() []components.V2Posting {
- postings := []components.V2Posting{}
-
- for range random.GetRandom()%20+1 {
+func RandomPostings() []components.V2Posting {
+ n := int(random.GetRandom()%20 + 1)
+ postings := make([]components.V2Posting, 0, n)
+ for i := 0; i < n; i++ {
source := internal.GetRandomAddress()
destination := internal.GetRandomAddress()
amount := internal.RandomBigInt()
asset := random.RandomChoice([]string{"USD/2", "EUR/2", "COIN"})
postings = append(postings, components.V2Posting{
Amount: amount,
Asset: asset,
Destination: destination,
Source: source,
})
}
return postings
}📝 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 RandomPostings() []components.V2Posting { | |
| postings := []components.V2Posting{} | |
| for range random.GetRandom()%20+1 { | |
| source := internal.GetRandomAddress() | |
| destination := internal.GetRandomAddress() | |
| amount := internal.RandomBigInt() | |
| asset := random.RandomChoice([]string{"USD/2", "EUR/2", "COIN"}) | |
| postings = append(postings, components.V2Posting{ | |
| Amount: amount, | |
| Asset: asset, | |
| Destination: destination, | |
| Source: source, | |
| }) | |
| } | |
| func RandomPostings() []components.V2Posting { | |
| n := int(random.GetRandom()%20 + 1) | |
| postings := make([]components.V2Posting, 0, n) | |
| for i := 0; i < n; i++ { | |
| source := internal.GetRandomAddress() | |
| destination := internal.GetRandomAddress() | |
| amount := internal.RandomBigInt() | |
| asset := random.RandomChoice([]string{"USD/2", "EUR/2", "COIN"}) | |
| postings = append(postings, components.V2Posting{ | |
| Amount: amount, | |
| Asset: asset, | |
| Destination: destination, | |
| Source: source, | |
| }) | |
| } | |
| return postings | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go around
lines 77 to 93, the loop using "for range random.GetRandom()%20+1" is invalid in
Go and the postings slice isn't preallocated; replace the invalid loop with a
conventional integer for-loop (e.g., compute n := int(random.GetRandom()%20) + 1
and use "for i := 0; i < n; i++") and preallocate the slice with capacity n
(make([]components.V2Posting, 0, n)) before appending to reduce allocations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
test/antithesis/Justfile (2)
25-27: Add preflight env checks for build inputs.Fail fast if required variables are missing.
requirements-build: - just config/push - earthly --push ./workload+build --ANTITHESIS_REPOSITORY="$ANTITHESIS_REPOSITORY" --LEDGER_LATEST_TAG="$LEDGER_LATEST_TAG" + : "${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY is required}" + : "${LEDGER_LATEST_TAG:?LEDGER_LATEST_TAG is required}" + just config/push + earthly --push ./workload+build --ANTITHESIS_REPOSITORY="$ANTITHESIS_REPOSITORY" --LEDGER_LATEST_TAG="$LEDGER_LATEST_TAG"
1-23: Optional: de-duplicate with a parameterized recipe.Reduce drift by having one run target with a duration parameter, e.g., run DURATION: requirements-build; then call just run 0.1 and just run 1.
-run-6min: requirements-build - # ... - # "custom.duration": "0.1", - # ... - -run-1h: requirements-build - # ... - # "custom.duration": "1", - # ... +run duration: requirements-build + # ... same curl as above ... + # Replace the duration field with: + # "custom.duration": "'"{{duration}}"'", \ + +# Convenience aliases +run-6min: run "0.1" +run-1h: run "1"test/antithesis/workload/bin/cmds/eventually_correct/main.go (1)
130-133: Don’t mutate tx ID from the response; copy big.Int before arithmetic
Mutating the pointer returned in Data[0].ID changes the underlying struct. Copy it first.- if expectedTxId == nil { - expectedTxId = transactions.V2TransactionsCursorResponse.Cursor.Data[0].ID - expectedTxId.Add(expectedTxId, big.NewInt(1)) - } + if expectedTxId == nil { + expectedTxId = new(big.Int).Set(transactions.V2TransactionsCursorResponse.Cursor.Data[0].ID) + expectedTxId.Add(expectedTxId, big.NewInt(1)) + }Based on learnings.
test/antithesis/workload/internal/utils.go (3)
23-28: Simplify RandomBigInt; avoid fmt conversions
Use SetUint64 for clarity and speed.-func RandomBigInt() *big.Int { - v := random.GetRandom() - ret := big.NewInt(0) - ret.SetString(fmt.Sprintf("%d", v), 10) - return ret -} +func RandomBigInt() *big.Int { + return new(big.Int).SetUint64(random.GetRandom()) +}
79-91: List all ledgers: add pagination
Current code returns only the first page. Iterate until HasMore is false.-func ListLedgers(ctx context.Context, client *client.Formance) ([]string, error) { - res, err := client.Ledger.V2.ListLedgers(ctx, operations.V2ListLedgersRequest{}) - if err != nil { - return nil, err - } - - ledgers := []string{} - for _, ledger := range res.V2LedgerListResponse.Cursor.Data { - ledgers = append(ledgers, ledger.Name) - } - - return ledgers, nil -} +func ListLedgers(ctx context.Context, client *client.Formance) ([]string, error) { + ledgers := []string{} + var cursor *string + for { + res, err := client.Ledger.V2.ListLedgers(ctx, operations.V2ListLedgersRequest{Cursor: cursor}) + if err != nil { + return nil, err + } + for _, l := range res.V2LedgerListResponse.Cursor.Data { + ledgers = append(ledgers, l.Name) + } + if !res.V2LedgerListResponse.Cursor.HasMore { + break + } + cursor = res.V2LedgerListResponse.Cursor.Next + } + return ledgers, nil +}
19-19: Prefer int for USER_ACCOUNT_COUNT to avoid repeated casts
Keeps loops simpler; adjust GetRandomAddress cast accordingly.-const USER_ACCOUNT_COUNT uint64 = 1000; +const USER_ACCOUNT_COUNT int = 1000Outside this hunk, update GetRandomAddress:
-func GetRandomAddress() string { - return random.RandomChoice([]string{"world", fmt.Sprintf("users:%d", random.GetRandom()%USER_ACCOUNT_COUNT)}) -} +func GetRandomAddress() string { + n := int(random.GetRandom()%uint64(USER_ACCOUNT_COUNT)) + return random.RandomChoice([]string{"world", fmt.Sprintf("users:%d", n)}) +}Also applies to: 108-110
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (1)
33-39: Optional: capture variables inside the closure
Avoid accidental races if outer variables change in future edits.- for i := 0; i < count; i++ { - pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) - } + for i := 0; i < count; i++ { + localCtx, localClient, localLedger := ctx, client, ledger + pool.Submit(func() { CreateTransaction(localCtx, localClient, localLedger) }) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
test/antithesis/Justfile(1 hunks)test/antithesis/workload/bin/cmds/eventually_correct/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/internal/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-20T13:08:10.034Z
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
📚 Learning: 2025-02-24T11:31:17.135Z
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:31-34
Timestamp: 2025-02-24T11:31:17.135Z
Learning: In Go, using range with an integer constant (e.g., `for i := range countLedgers`) is valid syntax and compiles correctly.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.gotest/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
🧬 Code graph analysis (3)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (7)
test/antithesis/workload/internal/utils.go (4)
NewClient(42-62)ListLedgers(79-91)Details(21-21)USER_ACCOUNT_COUNT(19-19)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-24)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(37-61)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (5)
test/antithesis/workload/internal/utils.go (5)
NewClient(42-62)GetRandomLedger(93-106)Details(21-21)GetRandomAddress(108-110)RandomBigInt(23-28)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumInsufficientFund(14-14)
test/antithesis/workload/internal/utils.go (4)
pkg/client/formance.go (3)
Formance(50-56)WithClient(90-94)WithRetryConfig(112-116)pkg/client/models/operations/v2createledger.go (2)
V2CreateLedgerResponse(29-31)V2CreateLedgerRequest(9-13)pkg/client/v2.go (1)
V2(20-24)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-24)
🔇 Additional comments (13)
test/antithesis/Justfile (3)
6-6: Confirm duration units with the API."0.1" likely means 0.1 hours (6 minutes) while "1" is 1 hour. Please confirm the API expects hours-as-string vs minutes/seconds.
Also applies to: 18-18
2-11: Stop leaking credentials; use netrc, set JSON header, and harden curl.Current command exposes secrets via --user, lacks Content-Type, and has no retries/timeouts. Use a temp netrc and standard resiliency flags.
- curl --fail \ - --user "formance:$ANTITHESIS_PASSWORD" \ - -X POST https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s -d '{ \ + # Preconditions & secure auth (avoid exposing secrets in process list) + : "${ANTITHESIS_PASSWORD:?ANTITHESIS_PASSWORD is required}" + : "${ANTITHESIS_SLACK_REPORT_RECIPIENT:?ANTITHESIS_SLACK_REPORT_RECIPIENT is required}" + tmp_netrc="$(mktemp)"; trap 'rm -f "$tmp_netrc"' EXIT + printf 'machine formance.antithesis.com login formance password %s\n' "$ANTITHESIS_PASSWORD" > "$tmp_netrc" + chmod 600 "$tmp_netrc" + curl --fail --silent --show-error --location --retry 3 --retry-all-errors --connect-timeout 10 --max-time 120 \ + --netrc-file "$tmp_netrc" \ + -H 'Content-Type: application/json' \ + -X POST 'https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s' -d '{ \
14-23: Mirror security and resiliency fixes in run-1h.Apply the same netrc, JSON header, and curl hardening here.
- curl --fail \ - --user "formance:$ANTITHESIS_PASSWORD" \ - -X POST https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s -d '{ \ + : "${ANTITHESIS_PASSWORD:?ANTITHESIS_PASSWORD is required}" + : "${ANTITHESIS_SLACK_REPORT_RECIPIENT:?ANTITHESIS_SLACK_REPORT_RECIPIENT is required}" + tmp_netrc="$(mktemp)"; trap 'rm -f "$tmp_netrc"' EXIT + printf 'machine formance.antithesis.com login formance password %s\n' "$ANTITHESIS_PASSWORD" > "$tmp_netrc" + chmod 600 "$tmp_netrc" + curl --fail --silent --show-error --location --retry 3 --retry-all-errors --connect-timeout 10 --max-time 120 \ + --netrc-file "$tmp_netrc" \ + -H 'Content-Type: application/json' \ + -X POST 'https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s' -d '{ \test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
112-119: Pagination logic for transactions looks good
Passing and advancing the cursor avoids the infinite loop on HasMore.Also applies to: 141-145
149-151: Remove duplicate IsServerError (use internal.IsServerError instead)
Local copy risks divergence and it’s unused here.-func IsServerError(httpMeta components.HTTPMetadata) bool { - return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 -}
44-61: Fix nil-deref and avoid killing the process from goroutines; early-return on errorGuard aggregated before GetHTTPMeta(), replace log.Fatalf with non-fatal logging, and return to avoid using aggregated after errors.
func checkBalanced(ctx context.Context, client *client.Formance, ledger string) { aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ Ledger: ledger, }) 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) - } + httpMeta := components.HTTPMetadata{} + if aggregated != nil { + httpMeta = aggregated.GetHTTPMeta() + } + if internal.IsServerError(httpMeta) { + assert.Always(false, + fmt.Sprintf("error getting aggregated balances for ledger %s: %s", ledger, err), + internal.Details{"error": err}) + return + } + log.Printf("error getting aggregated balances for ledger %s: %s", ledger, err) + return }
62-70: Assert details should include actual volume, not err (err is nil here)Report the measured value and context.
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, - }) + ), internal.Details{ + "ledger": ledger, + "asset": asset, + "actual_volume": volumes.String(), + }) }
75-81: Invalid loop: cannot range over an integer constant
Use a counted loop.-func checkAccountBalances(ctx context.Context, client *client.Formance, ledger string) { - for i := range internal.USER_ACCOUNT_COUNT { +func checkAccountBalances(ctx context.Context, client *client.Formance, ledger string) { + for i := 0; i < int(internal.USER_ACCOUNT_COUNT); i++ { address := fmt.Sprintf("users:%d", i)
82-86: Invert Sometimes condition and enrich details
Assert success (err == nil) and include the address for context.- assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ - "ledger": ledger, - "error": err, - }) + assert.Sometimes(err == nil, "Client can get account", internal.Details{ + "ledger": ledger, + "address": address, + "error": err, + })test/antithesis/workload/internal/utils.go (1)
64-66: Confirm intent: 4xx treated as “server error”?
If you only want 5xx, restrict to 500–599; otherwise consider renaming to IsHTTPError for clarity.test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (3)
24-29: Don’t return after falling back to the default ledger
Continue and submit work against "default".ledger, err := internal.GetRandomLedger(ctx, client) if err != nil { ledger = "default" log.Printf("error getting random ledger: %s", err) - return }
35-39: Fix invalid loop:for range countwon’t compile
Use a counted loop.- for range count { - pool.Submit(func() { - CreateTransaction(ctx, client, ledger) - }) - } + for i := 0; i < count; i++ { + pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) + }
77-95: Fix invalid loop and preallocate slice in RandomPostings
The current loop won’t compile; also preallocate to reduce allocations.func RandomPostings() []components.V2Posting { - postings := []components.V2Posting{} - - for range random.GetRandom()%2+1 { + n := int(random.GetRandom()%2 + 1) + postings := make([]components.V2Posting, 0, n) + for i := 0; i < n; i++ { source := internal.GetRandomAddress() destination := internal.GetRandomAddress() amount := internal.RandomBigInt() asset := random.RandomChoice([]string{"USD/2", "EUR/2", "COIN"}) postings = append(postings, components.V2Posting{ Amount: amount, Asset: asset, Destination: destination, Source: source, }) } return postings }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (3)
66-82: Replace the invalid postings loop with a counted loop.Line 69 uses
for range random.GetRandom()%2+1, which is not valid Go syntax and prevents the binary from compiling. Compute the count and loop with an index.-func RandomPostings() []components.V2Posting { - postings := []components.V2Posting{} - - for range random.GetRandom()%2+1 { +func RandomPostings() []components.V2Posting { + n := int(random.GetRandom()%2 + 1) + postings := make([]components.V2Posting, 0, n) + + for i := 0; i < n; i++ {
29-36: Fix invalid loop construct.
for range counton Line 33 does not compile;rangeexpects an iterable, not a bare integer. Use a counted loop instead.- for range count { + for i := 0; i < count; i++ { pool.Submit(func() { CreateTransaction(ctx, client, ledger) }) }
23-27: Keep running after falling back to the default ledger.Line 26 returns immediately after assigning
ledger = "default", so no workload runs when we hit the fallback path. Drop the return so the tasks still execute.if err != nil { ledger = "default" log.Printf("error getting random ledger: %s", err) - return }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
.github/workflows/antithesis_daily.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtest/antithesis/workload/go.modis excluded by!**/*.modtest/antithesis/workload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
Earthfile(1 hunks)internal/storage/driver/driver.go(4 hunks)test/antithesis/Justfile(1 hunks)test/antithesis/config/Justfile(1 hunks)test/antithesis/image/Dockerfile.ledger(1 hunks)test/antithesis/image/Justfile(1 hunks)test/antithesis/workload/Dockerfile.workload(1 hunks)test/antithesis/workload/Justfile(1 hunks)test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Earthfile
🚧 Files skipped from review as they are similar to previous changes (3)
- test/antithesis/config/Justfile
- test/antithesis/workload/bin/cmds/eventually_correct/main.go
- test/antithesis/Justfile
🧰 Additional context used
🧬 Code graph analysis (3)
internal/storage/driver/driver.go (4)
internal/ledger.go (1)
Ledger(18-26)internal/storage/ledger/store.go (1)
Store(22-44)internal/storage/system/store.go (1)
Store(20-31)internal/storage/bucket/bucket.go (1)
Bucket(13-21)
test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go (4)
test/antithesis/workload/internal/utils.go (5)
NewClient(42-62)GetRandomLedger(93-106)Details(21-21)GetRandomAddress(108-110)RandomBigInt(23-28)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)
test/antithesis/workload/internal/utils.go (4)
pkg/client/formance.go (3)
Formance(50-56)WithClient(90-94)WithRetryConfig(112-116)pkg/client/models/operations/v2createledger.go (2)
V2CreateLedgerResponse(29-31)V2CreateLedgerRequest(9-13)pkg/client/v2.go (1)
V2(20-24)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-24)
test/antithesis/workload/Justfile
Outdated
| mkdir -p out | ||
| go build -o ./out/init ./bin/init | ||
| for file in $(ls ./bin/cmds/); do \ | ||
| go build -o ./out/cmds/$file ./bin/cmds/$file; \ | ||
| done |
There was a problem hiding this comment.
Create the output directory for command binaries.
Line 21 writes go build -o ./out/cmds/$file, but ./out/cmds is never created, so the first build fails with “no such file or directory”. Please create the directory before the loop.
- mkdir -p out
+ mkdir -p out
+ mkdir -p out/cmds📝 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.
| mkdir -p out | |
| go build -o ./out/init ./bin/init | |
| for file in $(ls ./bin/cmds/); do \ | |
| go build -o ./out/cmds/$file ./bin/cmds/$file; \ | |
| done | |
| mkdir -p out | |
| mkdir -p out/cmds | |
| go build -o ./out/init ./bin/init | |
| for file in $(ls ./bin/cmds/); do \ | |
| go build -o ./out/cmds/$file ./bin/cmds/$file; \ | |
| done |
🤖 Prompt for AI Agents
In test/antithesis/workload/Justfile around lines 19 to 23, the recipe builds
command binaries into ./out/cmds but never creates that directory, causing the
first go build to fail; add a step before the loop to create the directory
(e.g., mkdir -p ./out/cmds) so the output path exists prior to running the
for-loop builds.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
test/antithesis/config/Justfile (1)
1-14: Add strict error handling and validate required environment variables.The
build-manifestrecipe lacks error handling and does not verify thatLEDGER_PREVIOUS_TAGis set before using it in theyqcommand. This can lead to silent failures or incorrect manifest generation.Apply this diff to add strict shell flags and environment variable validation:
build-manifest: + set -euo pipefail + : "$${LEDGER_PREVIOUS_TAG:?LEDGER_PREVIOUS_TAG is required}" rm -f -- tmp/resources.yaml mkdir -p tmp cat manifests/namespace.yaml >> tmp/resources.yaml echo "---" >> tmp/resources.yaml cat manifests/postgres.yaml >> tmp/resources.yaml echo "---" >> tmp/resources.yaml helm template regions oci://ghcr.io/formancehq/helm/regions --version 2.15.2 --namespace formance-systems >> tmp/resources.yaml echo "---" >> tmp/resources.yaml cat manifests/stack.yaml >> tmp/resources.yaml echo "---" >> tmp/resources.yaml yq '.spec.version = strenv(LEDGER_PREVIOUS_TAG)' manifests/ledger.yaml >> tmp/resources.yaml echo "---" >> tmp/resources.yaml cat manifests/workload.yaml >> tmp/resources.yamlNote: In Just recipes, you may need to prefix each line with the shell invocation or use a here-doc if Just doesn't propagate
setflags across lines.test/antithesis/Justfile (2)
1-11: Switch to netrc for credentials, add JSON header, and harden curl.The
run-6minrecipe exposes$ANTITHESIS_PASSWORDin process listings via--user, lacks a JSON content-type header, and has no retry or timeout flags for resilience.Apply this diff to address security and resilience concerns:
run-6min: requirements-push + : "$${ANTITHESIS_PASSWORD:?ANTITHESIS_PASSWORD is required}" + : "$${ANTITHESIS_SLACK_REPORT_RECIPIENT:?ANTITHESIS_SLACK_REPORT_RECIPIENT is required}" + tmp_netrc="$$(mktemp)"; trap 'rm -f "$$tmp_netrc"' EXIT + printf 'machine formance.antithesis.com login formance password %s\n' "$$ANTITHESIS_PASSWORD" > "$$tmp_netrc" + chmod 600 "$$tmp_netrc" - curl --fail \ - --user "formance:$ANTITHESIS_PASSWORD" \ - -X POST https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s -d '{ \ + curl --fail --silent --show-error --retry 3 --retry-all-errors \ + --netrc-file "$$tmp_netrc" \ + -H 'Content-Type: application/json' \ + -X POST 'https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s' -d '{ \ "params": { \ "custom.duration": "0.1", \ "antithesis.report.recipients": "'"$ANTITHESIS_SLACK_REPORT_RECIPIENT"'", \ "antithesis.config_image": "antithesis-config:daily_run", \ "antithesis.images": "workload:latest;docker.io/library/postgres:15-alpine;ghcr.io/formancehq/operator:v2.10.1;ghcr.io/formancehq/operator-utils:v2.0.14;ghcr.io/formancehq/gateway:v2.0.24;ghcr.io/formancehq/ledger-instrumented:'"$LEDGER_LATEST_TAG"'" \ } \ }'Note: Adjust the variable escaping (
$$) according to Just's syntax if needed.
13-23: Mirror the security and resiliency fixes in run-1h.The
run-1hrecipe has the same credential exposure and resilience issues asrun-6min.Apply this diff:
run-1h: requirements-push + : "$${ANTITHESIS_PASSWORD:?ANTITHESIS_PASSWORD is required}" + : "$${ANTITHESIS_SLACK_REPORT_RECIPIENT:?ANTITHESIS_SLACK_REPORT_RECIPIENT is required}" + tmp_netrc="$$(mktemp)"; trap 'rm -f "$$tmp_netrc"' EXIT + printf 'machine formance.antithesis.com login formance password %s\n' "$$ANTITHESIS_PASSWORD" > "$$tmp_netrc" + chmod 600 "$$tmp_netrc" - curl --fail \ - --user "formance:$ANTITHESIS_PASSWORD" \ - -X POST https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s -d '{ \ + curl --fail --silent --show-error --retry 3 --retry-all-errors \ + --netrc-file "$$tmp_netrc" \ + -H 'Content-Type: application/json' \ + -X POST 'https://formance.antithesis.com/api/v1/launch_experiment/formance-k8s' -d '{ \ "params": { \ "custom.duration": "1", \ "antithesis.report.recipients": "'"$ANTITHESIS_SLACK_REPORT_RECIPIENT"'", \ "antithesis.config_image": "antithesis-config:daily_run", \ "antithesis.images": "workload:latest;docker.io/library/postgres:15-alpine;ghcr.io/formancehq/operator:v2.10.1;ghcr.io/formancehq/operator-utils:v2.0.14;ghcr.io/formancehq/gateway:v2.0.24;ghcr.io/formancehq/ledger-instrumented:"'"$LEDGER_LATEST_TAG"'" \ } \ }'
🧹 Nitpick comments (2)
test/antithesis/image/Justfile (2)
6-6: Consider pinning the instrumentor version.Using
@latestcan lead to non-reproducible builds and unexpected breakage if the tool is updated with breaking changes.Apply this diff to pin to a specific version:
- go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest + go install github.com/antithesishq/antithesis-sdk-go/tools/[email protected]Note: Replace
v0.3.11with the desired version.
19-20: Validate prerequisites and environment variables.The script relies on
$LEDGER_TAGbeing set and Docker being installed and authenticated, but doesn't validate these prerequisites. Whileset -uwill catch undefinedLEDGER_TAG, explicit validation would provide clearer error messages and fail faster.Consider adding validation at the beginning of the script:
set -euxo pipefail + # Validate prerequisites + if [ -z "${LEDGER_TAG:-}" ]; then + echo "Error: LEDGER_TAG environment variable must be set" + exit 1 + fi + + if ! command -v docker &> /dev/null; then + echo "Error: docker is not installed" + exit 1 + fi + go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.github/workflows/antithesis_daily.ymlis excluded by!**/*.yml.github/workflows/antithesis_release.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
test/antithesis/Justfile(1 hunks)test/antithesis/config/Justfile(1 hunks)test/antithesis/image/Justfile(1 hunks)test/antithesis/workload/Justfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/workload/Justfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests
- GitHub Check: Dirty
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (4)
test/antithesis/image/Justfile (3)
1-2: LGTM!Recipe definition and shebang are appropriate for this orchestration task.
4-4: LGTM!Strict error handling with
set -euxo pipefailis appropriate for a build and push workflow, ensuring failures are caught early.
10-10: scriptdir=$(pwd) is valid
Just automatically executes recipes in the Justfile’s directory, so pwd reliably returns the script location.Likely an incorrect or invalid review comment.
test/antithesis/Justfile (1)
25-27: LGTM!The
requirements-pushrecipe correctly orchestrates the push of config and workload artifacts.
| push: build-manifest | ||
| docker build -f Dockerfile.config -t $ANTITHESIS_REPOSITORY/antithesis-config:daily_run . | ||
| docker push $ANTITHESIS_REPOSITORY/antithesis-config:daily_run |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate ANTITHESIS_REPOSITORY before use.
The push recipe uses $ANTITHESIS_REPOSITORY without verifying it is set. If unset, the Docker commands will fail with unclear error messages.
Apply this diff to add validation:
push: build-manifest
+ : "$${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY is required}"
docker build -f Dockerfile.config -t $ANTITHESIS_REPOSITORY/antithesis-config:daily_run .
docker push $ANTITHESIS_REPOSITORY/antithesis-config:daily_run📝 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.
| push: build-manifest | |
| docker build -f Dockerfile.config -t $ANTITHESIS_REPOSITORY/antithesis-config:daily_run . | |
| docker push $ANTITHESIS_REPOSITORY/antithesis-config:daily_run | |
| push: build-manifest | |
| : "$${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY is required}" | |
| docker build -f Dockerfile.config -t $ANTITHESIS_REPOSITORY/antithesis-config:daily_run . | |
| docker push $ANTITHESIS_REPOSITORY/antithesis-config:daily_run |
🤖 Prompt for AI Agents
In test/antithesis/config/Justfile around lines 16 to 18, the push recipe uses
$ANTITHESIS_REPOSITORY without validating it; add a guard at the start of the
push recipe that checks the variable is set and non-empty and fails with a clear
error if not. Specifically, prepend a test like: if [ -z
"$ANTITHESIS_REPOSITORY" ]; then echo "ANTITHESIS_REPOSITORY must be set"; exit
1; fi — then proceed with the docker build and push commands so the recipe fails
fast with a helpful message instead of producing unclear Docker errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/antithesis/workload/Justfile (1)
20-24: Create the output directory for command binaries.Line 23 writes to
./out/cmds/$file, but./out/cmdsis never created. The first build will fail with "no such file or directory".Apply this diff:
mkdir -p out + mkdir -p out/cmds go build -o ./out/init ./bin/init
🧹 Nitpick comments (2)
test/antithesis/workload/Justfile (2)
17-17: Pin the instrumentor version for reproducible builds.Using
@latestmakes builds non-reproducible. Consider pinning to a specific version or commit hash.Example:
- GOBIN=$(pwd) go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest + GOBIN=$(pwd) go install github.com/antithesishq/antithesis-sdk-go/tools/[email protected]
26-29: Validate required environment variables.The script relies on
LEDGER_LATEST_TAGandANTITHESIS_REPOSITORYbut doesn't validate they are set. If empty, this could lead to silent failures or confusing error messages.Add validation at the beginning of the script (after line 7):
scriptdir=$(pwd) : "${LEDGER_LATEST_TAG:?LEDGER_LATEST_TAG must be set}" : "${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY must be set}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.github/workflows/antithesis_daily.ymlis excluded by!**/*.yml.github/workflows/antithesis_release.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
test/antithesis/image/Justfile(1 hunks)test/antithesis/workload/Justfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/image/Justfile
test/antithesis/workload/Justfile
Outdated
|
|
||
| set -euxo pipefail | ||
| tmpdir=$(mktemp -d) | ||
| trap 'rm -rf $tmpdir' EXIT |
There was a problem hiding this comment.
Quote the variable in the trap command.
The trap uses $tmpdir without quotes. If the temporary directory path contains spaces, the cleanup will fail.
Apply this diff:
- trap 'rm -rf $tmpdir' EXIT
+ trap 'rm -rf "$tmpdir"' EXIT📝 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.
| trap 'rm -rf $tmpdir' EXIT | |
| trap 'rm -rf "$tmpdir"' EXIT |
🤖 Prompt for AI Agents
In test/antithesis/workload/Justfile around line 6, the trap currently uses
$tmpdir unquoted which will break if the temporary path contains spaces; update
the trap command so the variable is quoted (i.e., ensure rm -rf is invoked with
"$tmpdir" quoted inside the trap command) so cleanup works for paths with
spaces.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/antithesis/workload/Dockerfile.workload (4)
4-4: Harden and slim the apt install.Use no‑recommends, install CA certs, and clean apt lists to reduce image size.
Apply this diff:
-RUN apt-get update && apt-get install -y curl +RUN apt-get update \ + && apt-get install -y --no-install-recommends ca-certificates curl \ + && rm -rf /var/lib/apt/lists/*
6-7: Ensure/initis executable; consider non‑root runtime.Make the binary executable; optionally drop root for better container security.
Apply this diff:
COPY init /init +RUN chmod +x /init ENTRYPOINT ["/init"]Optional (non-root user) — add after package install:
# Create non-root user and switch RUN useradd -r -u 10001 -g nogroup appuser USER 10001
9-9: Prefer directory copy over wildcard.
COPY cmds/*can miss dotfiles and fail on empty matches. Copy the directory recursively; add --chown if switching to non-root.Apply this diff:
-COPY cmds/* /opt/antithesis/test/v1/main/ +COPY cmds/ /opt/antithesis/test/v1/main/If you adopt a non-root user:
-COPY cmds/ /opt/antithesis/test/v1/main/ +COPY --chown=appuser:nogroup cmds/ /opt/antithesis/test/v1/main/
11-11: Confirm intent forledger_latest_tag.If it’s only metadata, consider passing it as
ARG LEDGER_LATEST_TAG+ENVorLABELinstead of copying a file.Example:
ARG LEDGER_LATEST_TAG ENV LEDGER_LATEST_TAG=${LEDGER_LATEST_TAG} # or LABEL org.opencontainers.image.version=${LEDGER_LATEST_TAG}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/antithesis/image/Justfile(1 hunks)test/antithesis/workload/Dockerfile.workload(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/image/Justfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Trigger daily run
| @@ -0,0 +1,11 @@ | |||
| FROM ubuntu:latest | |||
There was a problem hiding this comment.
Pin the base image (avoid latest).
Floating tags hurt reproducibility and security posture. Pin to a specific version (and ideally a digest).
Apply this diff:
-FROM ubuntu:latest
+FROM ubuntu:22.04📝 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.
| FROM ubuntu:latest | |
| FROM ubuntu:22.04 |
🤖 Prompt for AI Agents
In test/antithesis/workload/Dockerfile.workload around line 1, the base image is
using the floating tag "ubuntu:latest"; replace it with a specific, immutable
tag or digest (e.g., ubuntu:22.04 or ubuntu@sha256:<digest>) to ensure
reproducibility and improve security — update the FROM line to reference the
chosen stable version or digest and commit the change.
| COPY init /init | ||
| ENTRYPOINT ["/init"] | ||
|
|
||
| COPY cmds/* /opt/antithesis/test/v1/main/ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify build artifact paths (cmds/ vs bin/cmds/) and tag file presence.
The workload build may output binaries under bin/cmds/, not cmds/. Also ensure ledger_latest_tag exists at build time.
Run this script to confirm paths:
Also applies to: 11-11
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check for workload command outputs:"
fd -t d -a 'cmds' | sed 's/^/DIR: /'
fd -t f -a 'cmds' -E '**/vendor/**' | sed 's/^/FILE: /' | head -n 20
echo
echo "Check for bin/cmds:"
fd -t d -a 'bin/cmds' | sed 's/^/DIR: /'
fd -t f -a 'bin/cmds' -E '**/vendor/**' | sed 's/^/FILE: /' | head -n 20
echo
echo "Check ledger_latest_tag presence near Dockerfile:"
fd -a 'ledger_latest_tag' -g 'test/antithesis/**' | sed 's/^/FOUND: /'Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check for ledger_latest_tag in test/antithesis/workload:"
fd -t f -a 'ledger_latest_tag' test/antithesis/workloadLength of output: 56
Correct workload COPY paths and include ledger_latest_tag
In test/antithesis/workload/Dockerfile.workload (lines 9 & 11) replace
COPY cmds/* /opt/antithesis/test/v1/main/with
COPY bin/cmds/* /opt/antithesis/test/v1/main/and add a step to copy your ledger_latest_tag into the image (e.g.
COPY ledger_latest_tag /opt/antithesis/test/v1/main/ledger_latest_tag
```) so the tag file is present at build time.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In test/antithesis/workload/Dockerfile.workload around lines 9 and 11, the
Dockerfile copies from cmds/* which is the wrong source path and it does not
include the ledger_latest_tag file; change the COPY source to bin/cmds/* so
binaries are copied from bin/cmds into /opt/antithesis/test/v1/main/, and add an
additional COPY instruction to copy ledger_latest_tag into
/opt/antithesis/test/v1/main/ledger_latest_tag so the tag file is present at
build time.
</details>
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/storage/driver/driver.go (1)
81-91: Unconditional assertion still trips every outdated-bucket path.We’re still calling
assert.AlwaysOrUnreachable(false, …)with a literalfalse, so the assertion fires every time we hit the outdated-bucket path. The earlier review already flagged this; the TODO still needs to be resolved by gating the assertion behind the actual “new binary version” flag (or removing it). As-is, the DST agent treats this branch as unreachable in all scenarios and reports a failure even for expected rollouts.Please replace the hard-coded
falsewith the real flag (or another proper condition) and cover both cases with tests.
🧹 Nitpick comments (6)
test/antithesis/workload/Justfile (1)
1-3: Validate required environment variables.The recipe depends on
$LEDGER_LATEST_TAGand$ANTITHESIS_REPOSITORYbeing set but does not validate their presence. If either is unset, Docker will silently use empty strings, producing broken tags or pushing to invalid repositories.Consider adding validation at the start of the recipe:
push: + @test -n "$$LEDGER_LATEST_TAG" || (echo "LEDGER_LATEST_TAG is required" && exit 1) + @test -n "$$ANTITHESIS_REPOSITORY" || (echo "ANTITHESIS_REPOSITORY is required" && exit 1) docker build --build-arg LEDGER_LATEST_TAG="$LEDGER_LATEST_TAG" -f Dockerfile -t "$ANTITHESIS_REPOSITORY/workload:latest" ../../../ docker push "$ANTITHESIS_REPOSITORY/workload:latest"test/antithesis/image/Dockerfile (3)
4-4: Clean up apt cache to reduce image size.The
apt-get updateinstalls packages but leaves the apt cache, unnecessarily inflating the layer size.Apply this diff:
-RUN apt-get update && apt-get install -y bash git gcc +RUN apt-get update && apt-get install -y bash git gcc && rm -rf /var/lib/apt/lists/*
16-16: Pin the instrumentor version for reproducibility.Using
@latestfor the Antithesis instrumentor means builds are not reproducible and may break unexpectedly when the tool is updated.Pin 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/[email protected]Replace
v0.x.xwith the current stable version.
26-26: Clean up apt cache in runner stage.Similar to the compiler stage, the apt cache should be cleaned to reduce the final image size.
Apply this diff:
-RUN apt-get update && apt-get install -y postgresql-client curl +RUN apt-get update && apt-get install -y postgresql-client curl && rm -rf /var/lib/apt/lists/*test/antithesis/image/Justfile (1)
1-3: Validate required environment variable.The recipe depends on
$LEDGER_TAGbeing set but does not validate its presence. If unset, Docker will use an empty string, producing a malformed tag likeghcr.io/formancehq/ledger-instrumented:.Add validation at the start of the recipe:
push: + @test -n "$$LEDGER_TAG" || (echo "LEDGER_TAG is required" && exit 1) docker build -f Dockerfile -t "ghcr.io/formancehq/ledger-instrumented:$LEDGER_TAG" ../../../ docker push "ghcr.io/formancehq/ledger-instrumented:$LEDGER_TAG"test/antithesis/workload/Dockerfile (1)
4-4: Pin the instrumentor version for reproducibility.Using
@latestmeans builds are not reproducible and may break when the tool is updated.Pin 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/[email protected]Replace
v0.x.xwith the current stable version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/storage/driver/driver.go(4 hunks)test/antithesis/image/Dockerfile(1 hunks)test/antithesis/image/Justfile(1 hunks)test/antithesis/workload/Dockerfile(1 hunks)test/antithesis/workload/Justfile(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/storage/driver/driver.go (4)
internal/controller/system/store.go (2)
Driver(20-24)Store(13-18)internal/ledger.go (1)
Ledger(18-26)internal/storage/system/store.go (1)
Store(20-31)internal/storage/bucket/bucket.go (1)
Bucket(13-21)
🔇 Additional comments (1)
test/antithesis/workload/Dockerfile (1)
13-18: Clarify why instrumentor runs twice.The instrumentor is invoked with
-assert_onlybefore building (Line 13) and again after building (Line 18). If both invocations are necessary, consider adding comments explaining the purpose of each run. If the second invocation is redundant, it should be removed.Please confirm whether both instrumentor runs are intentional and document their purpose if so.
| COPY --from=compiler /instrumented/symbols/ /symbols | ||
| COPY test/antithesis/image/entrypoint.sh /bin/entrypoint.sh | ||
|
|
||
| RUN chmod 777 /bin/entrypoint.sh |
There was a problem hiding this comment.
Reduce permissions on the entrypoint script.
chmod 777 grants write and execute permissions to all users, which is a security risk. The script only needs to be executable by the container user.
Apply this diff:
-RUN chmod 777 /bin/entrypoint.sh
+RUN chmod 755 /bin/entrypoint.sh🤖 Prompt for AI Agents
In test/antithesis/image/Dockerfile around line 32, replace the unsafe chmod 777
on /bin/entrypoint.sh with a more restrictive permission (e.g., chmod 755 or
750) so the script is executable by the container user but not writable by
everyone; if needed also ensure the file ownership is appropriate (chown
root:root or to the intended runtime user) before setting permissions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (2)
58-66: Incorrect error variable in assert details.The
errvariable is nil at this point since this code executes after the error check. Include the actual volume value instead to provide more meaningful assertion details.Apply this diff to fix:
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, + "actual_volume": volumes.String(), + "asset": asset, + "ledger": ledger, }) }
78-81: Invert Sometimes condition.The assertion message says "Client can aggregate account balances" but checks
err != nil, which fires when there's an error. This should assert success (err == nil).Apply this diff:
- assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ + assert.Sometimes(err == nil, "Client can get account", internal.Details{ "ledger": ledger, + "address": address, "error": err, })
🧹 Nitpick comments (8)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (3)
17-19: Avoid high collision rate in ledger IDsUsing
%1e6makes collisions likely under parallel runs. Use the full 64-bit space.- id := random.GetRandom()%1e6 - ledger := fmt.Sprintf("ledger-%d", id) + id := random.GetRandom() + ledger := fmt.Sprintf("ledger-%d", id)
27-29: Add more context to assertion and clarify intentInclude the ledger name to aid debugging and make the intent explicit that collisions are expected.
- assert.Sometimes(err == nil, "ledger should have been created properly", internal.Details{ - "error": err, - }) + assert.Sometimes(err == nil, "ledger creation should sometimes succeed (collisions expected)", internal.Details{ + "error": err, + "ledger": ledger, + })
15-25: Consider bounding the operation with a timeoutFor CLI workloads, a short timeout helps bound hangs if the gateway is unreachable.
Example:
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel()Requires importing the standard time package in this file.
test/antithesis/workload/internal/utils.go (5)
23-28: Simplify RandomBigInt: avoid fmt and string parsingUse
SetUint64directly; cheaper and clearer.-func RandomBigInt() *big.Int { - v := random.GetRandom() - ret := big.NewInt(0) - ret.SetString(fmt.Sprintf("%d", v), 10) - return ret -} +func RandomBigInt() *big.Int { + return new(big.Int).SetUint64(random.GetRandom()) +}
35-39: Keep error typed in detailsPassing
errdirectly avoids unnecessary stringification; nil stays nil.- return AssertAlways(err == nil, message, Details{ - "error": fmt.Sprint(err), - "details": details, - }) + return AssertAlways(err == nil, message, Details{ + "error": err, + "details": details, + })
12-12: Optional: alias go-libs time import to avoid confusion with stdlibHelps readability since the identifier is
time.Minuteon an http.Client.- "github.com/formancehq/go-libs/v2/time" + ftime "github.com/formancehq/go-libs/v2/time"- Timeout: time.Minute, + Timeout: ftime.Minute,Also applies to: 49-51
75-87: ListLedgers ignores pagination; only first page is usedDefault page size is limited (15 per docs). Sampling from the first page biases selection and may miss most ledgers.
- Iterate pages using the cursor until exhaustion, aggregating names, or
- Randomly select a page first, then pick within that page.
Based on relevant code snippet indicating page size limits.
42-61: Retry config vs non-idempotent createCreateLedger is not idempotent; retries on connection errors can yield 409 Already Exists if the first attempt succeeded server-side. This is fine for the workload but ensure this is intentional.
If unintended, disable retries for POSTs or gate retries on idempotent methods only.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-24T11:31:17.135Z
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:31-34
Timestamp: 2025-02-24T11:31:17.135Z
Learning: In Go, using range with an integer constant (e.g., `for i := range countLedgers`) is valid syntax and compiles correctly.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
🧬 Code graph analysis (3)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (1)
test/antithesis/workload/internal/utils.go (3)
NewClient(42-62)CreateLedger(64-73)Details(21-21)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (6)
test/antithesis/workload/internal/utils.go (3)
NewClient(42-62)Details(21-21)USER_ACCOUNT_COUNT(19-19)pkg/client/v2.go (1)
V2(20-24)pkg/client/formance.go (1)
Formance(50-56)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)
test/antithesis/workload/internal/utils.go (4)
pkg/client/formance.go (3)
Formance(50-56)WithClient(90-94)WithRetryConfig(112-116)pkg/client/models/operations/v2createledger.go (2)
V2CreateLedgerResponse(29-31)V2CreateLedgerRequest(9-13)pkg/client/v2.go (1)
V2(20-24)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-24)
🔇 Additional comments (2)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (2)
16-40: LGTM!The main function correctly lists ledgers, handles errors with appropriate assertions, and spawns concurrent health checks with proper WaitGroup synchronization. The goroutine properly captures
ledger.Namein the function parameter to avoid closure issues.
85-101: LGTM!The balance consistency checks correctly validate that:
- Reported balance equals input minus output
- Balances remain non-negative when overdraft is not allowed
The assertions include comprehensive details for debugging.
| randomIndex := random.GetRandom()%uint64(len(ledgers)) | ||
|
|
||
| return ledgers[randomIndex], nil |
There was a problem hiding this comment.
Compile error: slice index type mismatch (uint64 vs int)
randomIndex is uint64; slice indices must be int.
- randomIndex := random.GetRandom()%uint64(len(ledgers))
-
- return ledgers[randomIndex], nil
+ idx := int(random.GetRandom() % uint64(len(ledgers)))
+ return ledgers[idx], nil📝 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.
| randomIndex := random.GetRandom()%uint64(len(ledgers)) | |
| return ledgers[randomIndex], nil | |
| idx := int(random.GetRandom() % uint64(len(ledgers))) | |
| return ledgers[idx], nil |
🤖 Prompt for AI Agents
In test/antithesis/workload/internal/utils.go around lines 99 to 101, the code
uses a uint64 randomIndex to index a slice (ledgers) which requires an int;
change the index to an int by converting the result before using it (for example
compute the modulus then cast to int, or generate an int index directly) and use
that int to access ledgers[randomIndex]. Ensure the conversion happens on the
value used for indexing to satisfy the slice index type.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/storage/driver/driver.go (1)
81-92: Previously flagged: Assertion needs proper flag control.This assertion logic has already been reviewed. The hardcoded
falseand missing test coverage were flagged in previous comments.
🧹 Nitpick comments (5)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
23-25: Assertion message should describe the success case.The condition checks
err == nil(success), but the message says "error listing ledgers". For Sometimes assertions, the message should describe what succeeds when the condition is true.Apply this diff:
- assert.Sometimes(err == nil, "error listing ledgers", internal.Details { + assert.Sometimes(err == nil, "Client can list ledgers", internal.Details{ "error": err, })
79-83: Fix assertion message to match the operation.The message says "aggregate account balances" but the operation is
GetAccount. The message should accurately describe what the API call does.Apply this diff:
- assert.Sometimes(err == nil, "Client can aggregate account balances", internal.Details{ + assert.Sometimes(err == nil, "Client can get account", internal.Details{ "ledger": ledger, "address": address, "error": err,
97-102: Assertion message should say "non-negative" instead of "positive".The condition checks for
>= 0, which includes zero. In mathematics, zero is not positive—it's non-negative. The message should accurately reflect the condition.Apply this diff:
- assert.Always(volume.Balance.Cmp(zero) != -1, "Balance should stay positive when no overdraft is allowed", internal.Details{ + assert.Always(volume.Balance.Cmp(zero) != -1, "Balance should be non-negative when no overdraft is allowed", internal.Details{ "ledger": ledger, "address": address, "asset": asset,
104-104: Remove trailing empty line inside the loop.Minor formatting cleanup.
Apply this diff:
} - }internal/storage/driver/driver.go (1)
41-54: Consider condensing the doc comment.The detailed step-by-step breakdown largely mirrors the code flow. Based on past feedback, consider keeping only the essential information: the function's purpose and the atomicity guarantee.
Apply this diff to condense the comment:
-/* -CreateLedger creates a new ledger in the system and sets up all necessary database objects. - -The function follows these steps: - 1. Create a ledger record in the system store (_system.ledgers table) - 2. Get the bucket (database schema) for this ledger - 3. Check if the bucket is already initialized: - a. If initialized: Verify it's up to date and add ledger-specific objects to it - b. If not initialized: Create the bucket schema with all necessary tables - 4. Return a ledger store that provides an interface to interact with the ledger - -Note: This entire process is wrapped in a database transaction, ensuring atomicity. -If any step fails, the entire transaction is rolled back, preventing partial state. -*/ +// CreateLedger atomically creates a new ledger, initializing or updating its bucket schema as needed. +// The entire operation runs in a transaction; any failure rolls back all changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
.github/workflows/antithesis_daily.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sumtools/provisioner/go.modis excluded by!**/*.modtools/provisioner/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
internal/storage/driver/driver.go(4 hunks)test/antithesis/Justfile(1 hunks)test/antithesis/workload/Dockerfile(1 hunks)test/antithesis/workload/bin/cmds/eventually_correct/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.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 (2)
- test/antithesis/Justfile
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-24T11:31:17.135Z
Learnt from: gfyrag
PR: formancehq/ledger#703
File: internal/storage/driver/driver_test.go:31-34
Timestamp: 2025-02-24T11:31:17.135Z
Learning: In Go, using range with an integer constant (e.g., `for i := range countLedgers`) is valid syntax and compiles correctly.
Applied to files:
test/antithesis/workload/bin/cmds/eventually_correct/main.go
🧬 Code graph analysis (2)
internal/storage/driver/driver.go (3)
internal/ledger.go (1)
Ledger(18-26)internal/storage/system/store.go (1)
Store(20-31)internal/storage/bucket/bucket.go (1)
Bucket(13-21)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
test/antithesis/workload/internal/utils.go (3)
NewClient(42-62)Details(21-21)USER_ACCOUNT_COUNT(19-19)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (7)
test/antithesis/workload/Dockerfile (4)
16-17: Directory creation and build loop are correct.The
/cmdsdirectory is now properly created before the build loop, and the binaries are correctly compiled to/cmds/$file. This addresses the prior critical issue.
28-31: COPY paths and entrypoint configuration are correct.The absolute path
/cmds/*is now correctly used in the COPY command, and the/initbinary is properly set as the entrypoint. These changes address prior critical issues.
33-33: Echo command syntax is correct.The redirect operator
>is now present, correctly writingLEDGER_LATEST_TAGto/ledger_latest_tag. This addresses the prior syntax error.
4-4: Pin antithesis-go-instrumentor to a specific version instead of @latest, and add -instrumentor_version flag to instrumentor invocations.Using
@latestcreates non-deterministic builds. However, the suggested version (v0.5.0) could not be confirmed as the current release; web search shows v0.4.4 (July 2025) as the latest. Since your codebase's go.mod already specifies SDK v0.5.0, verify whether v0.5.0 is available and correct, or whether you should align to the latest stable (v0.4.4). Also add the-instrumentor_versionflag to both instrumentor invocations (lines 13 and 19) to ensure alignment with the SDK version.Apply to line 4:
-RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest +RUN go install github.com/antithesishq/antithesis-sdk-go/tools/[email protected]Apply to lines 13 and 19:
-RUN antithesis-go-instrumentor -assert_only . +RUN antithesis-go-instrumentor -assert_only -instrumentor_version v0.5.0 .internal/storage/driver/driver.go (3)
8-8: LGTM: Antithesis assertion support added.The import is correctly placed to support the assertion at lines 81-92. Based on learnings, ensure your go.mod pins antithesis-sdk-go to v0.5.0 or later and that the Go instrumentor runs before build to catalog assertions.
58-58: Transaction wrapper ensures atomicity.Using
nilfor the isolation level applies the default (READ COMMITTED), which is appropriate here. The constraint check on line 62 correctly handles concurrent ledger creation attempts.
61-66: Verify whether doubleResolveErrorcall is necessary.The
postgres.ResolveErrorfunction is from an external dependency (github.com/formancehq/go-libs/v3 v3.0.1), and I cannot access its implementation. However, the code pattern at lines 63 and 65 callspostgres.ResolveError(err)twice on the same error object:
- Line 63:
postgres.ResolveError(err)to check if it's a constraint failure- Line 65:
postgres.ResolveError(err)again if the constraint check failsConsider storing the resolved error from line 63 and reusing it at line 65 to avoid potential redundant transformation, unless double resolution is intentional or the function performs necessary side effects on each call.
No description provided.