feat: add DST setup #879
feat: add DST setup #879
Conversation
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
WalkthroughCreateLedger now runs entirely inside a DB transaction performing ledger insert, bucket retrieval/init/migration/add, and ledger-store creation; transaction errors are resolved after rollback. Numerous Antithesis test artifacts were added: Earthfiles, entrypoint, workload binaries/utilities, Justfiles, and a minor import reorder. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Driver
participant DB as DB Tx (RunInTx)
participant System as SystemStore
participant BF as BucketFactory
participant Bucket
participant LSF as LedgerStoreFactory
Caller->>Driver: CreateLedger(ctx, ledger)
Driver->>DB: RunInTx {begin}
DB->>System: insert ledger record
alt constraint violation
System-->>DB: constraint error
DB-->>Driver: rollback (ErrLedgerAlreadyExists)
Driver-->>Caller: error
else other error
System-->>DB: error
DB-->>Driver: rollback (resolved error)
Driver-->>Caller: error
end
DB->>BF: Create(bucketName)
BF-->>DB: Bucket
DB->>Bucket: IsInitialized?
alt initialized
DB->>Bucket: IsUpToDate?
alt outdated
note right of Driver #f9f9c5: assert and return ErrBucketOutdated
DB-->>Driver: rollback (ErrBucketOutdated)
Driver-->>Caller: error
else up-to-date
DB->>Bucket: AddLedger(ledger)
end
else not initialized
DB->>Bucket: Migrate()
end
DB->>LSF: NewStore(Bucket, ledger)
LSF-->>DB: store
DB-->>Driver: commit
Driver-->>Caller: store
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #879 +/- ##
==========================================
- Coverage 80.62% 80.56% -0.06%
==========================================
Files 186 186
Lines 10207 10324 +117
==========================================
+ Hits 8229 8318 +89
- Misses 1559 1582 +23
- Partials 419 424 +5 ☔ 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.
| CACHE --id go-mod-cache /go/pkg/mod | ||
| CACHE --id go-cache /root/.cache/go-build | ||
|
|
||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the version of antithesis-go-instrumentor tool.
Using @latest for tools can lead to inconsistent builds and unexpected breaking changes. Consider pinning to a specific version.
- RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest
+ RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@v0.5.2 # Use appropriate version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest | |
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@v0.5.2 # Use appropriate version |
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
| compile: | ||
| FROM --platform=linux/amd64 golang:1.23 | ||
| CACHE --sharing=shared --id go-mod-cache /go/pkg/mod | ||
| CACHE --sharing=shared --id go-cache /root/.cache/go-build | ||
|
|
||
| COPY ../../..+sources/src /src | ||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest | ||
| WORKDIR /src | ||
|
|
||
| RUN go mod download | ||
| RUN mkdir -p /ledger_instrumented | ||
| RUN /go/bin/antithesis-go-instrumentor . /ledger_instrumented | ||
|
|
||
| WORKDIR /ledger_instrumented/customer | ||
| RUN go build -race -o ledger | ||
|
|
||
| SAVE ARTIFACT /ledger_instrumented/customer/ledger | ||
| SAVE ARTIFACT /ledger_instrumented/symbols | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check Go version
The compile stage uses golang:1.23, but the latest stable Go version is 1.22.x. This appears to be a non-existent future version.
- FROM --platform=linux/amd64 golang:1.23
+ FROM --platform=linux/amd64 golang:1.22🌐 Web query:
What is the latest stable version of Go?
💡 Result:
The latest stable version of Go as of April 2025 is Go 1.24.2, released on April 1, 2025[1][2]. This release includes security fixes to the net/http package as well as various bug fixes to the compiler, runtime, go command, and several libraries such as crypto/tls, go/types, net/http, and testing[2].
Citations:
- 1: https://go.dev/dl/
- 2: https://go.dev/doc/devel/release
- 3: https://tip.golang.org/doc/go1.24
- 4: https://github.com/golang/tools/releases
- 5: https://antonz.org/which-go/
- 6: https://tip.golang.org/doc/go1.23
- 7: https://discussion.fedoraproject.org/t/fedora-doesnt-provide-latest-version-of-go/146930
- 8: https://endoflife.date/go
Bump Go base image to latest stable (1.24)
The compile stage currently uses an outdated Go image (golang:1.23), but the latest stable release as of April 2025 is Go 1.24.2. To include the latest security and bug fixes, please update the Dockerfile:
File: test/antithesis/image/Earthfile
Lines: 10–28
- FROM --platform=linux/amd64 golang:1.23
+ FROM --platform=linux/amd64 golang:1.24If you prefer explicit patch‑level pinning, you can use:
FROM --platform=linux/amd64 golang:1.24.2
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 <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
|
|
||
| // 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 ^^
| @@ -0,0 +1,8 @@ | |||
| VERSION 0.7 | |||
There was a problem hiding this comment.
Stop using Earthfile, there is no Earthfile anymore in the repsoitory ;)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go (2)
12-28: The ledger creation appears correct, but consider adding confirmation logging.The code properly creates a ledger with a randomized name, but it would be helpful to add a success log message to confirm when the ledger has been created successfully. This would make troubleshooting easier when reviewing test logs.
if err != nil { assert.Always(err == nil, "ledger should have been created properly", internal.Details{ "error": err, }) return } + // Print confirmation that ledger was created successfully + fmt.Printf("Ledger '%s' created successfully\n", ledger) }
15-16: Consider checking for overflow when converting to int64.Using the absolute value of a randomly generated big integer and directly converting to int64 could potentially lead to overflow issues if the value exceeds the maximum int64 value.
- id := big.NewInt(0).Abs(internal.RandomBigInt()).Int64() + // Ensure the ID fits within int64 bounds + bigInt := big.NewInt(0).Abs(internal.RandomBigInt()) + // Use Mod to ensure it's within int64 range + maxInt64 := big.NewInt(math.MaxInt64) + id := big.NewInt(0).Mod(bigInt, maxInt64).Int64() ledger := fmt.Sprintf("ledger-%d", id)test/antithesis/workload/bin/cmds/first_default_ledger/main.go (1)
10-25: Consider adding confirmation logging.The code properly creates the default ledger, but lacks any success message that would be helpful for logging and debugging purposes.
if err != nil { assert.Always(err == nil, "ledger should have been created properly", internal.Details{ "error": err, }) return } + fmt.Println("Default ledger created successfully") }Don't forget to add the fmt import if you implement this change.
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
14-24: Add error handling for the edge case of no ledgers.The program should handle the case where no ledgers are returned, which is possible especially during initial setup.
ledgers, err := client.Ledger.V2.ListLedgers(ctx, operations.V2ListLedgersRequest{}) if err != nil { assert.Always(err == nil, "error listing ledgers", internal.Details{ "error": err, }) return } + // Handle the case of no ledgers found + if len(ledgers.V2LedgerListResponse.Cursor.Data) == 0 { + fmt.Println("No ledgers found to check volumes") + return + }test/antithesis/workload/internal/utils.go (3)
22-27: Add more context and validation to RandomBigInt function.The function relies on an external random number generator without validation or documentation about what it returns. Consider adding more context to make the function more robust.
func RandomBigInt() *big.Int { + // random.GetRandom() returns a 64-bit random integer v := random.GetRandom() ret := big.NewInt(0) ret.SetString(fmt.Sprintf("%d", v), 10) + // Ensure the returned value is non-negative as a safeguard + if ret.Sign() < 0 { + ret.Neg(ret) + } return ret }
81-96: Seed the random generator for better randomization.The
rand.Intn()function is used without seeding the random generator, which can lead to predictable sequences if called in quick succession. Consider seeding the random generator for better randomization.func GetRandomLedger(ctx context.Context, client *client.Formance) (string, error) { ledgers, err := ListLedgers(ctx, client) if err != nil { return "", err } if len(ledgers) == 0 { return "", fmt.Errorf("no ledgers found") } // Get a random index within the ledgers slice + // Seed the random generator with current time to ensure randomness + rand.Seed(time.Now().UnixNano()) randomIndex := rand.Intn(len(ledgers)) // Return the ledger at the random index return ledgers[randomIndex], nil }Note: If you're using Go 1.20+, you should use the newer
rand.New(rand.NewSource(time.Now().UnixNano()))approach instead, asrand.Seedis deprecated.
98-114: Hardcoded asset type in RunTx function.The function hardcodes "USD/2" as the asset, which may not be flexible enough for all test scenarios. Consider making the asset type configurable or adding documentation explaining why this specific asset is used.
-func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string) error { +// RunTx creates a transaction on the specified ledger with the given amount of the default asset (USD/2) +// If you need to use a different asset, use the RunTxWithAsset function instead +func RunTx(ctx context.Context, client *client.Formance, amount *big.Int, ledger string) error { + return RunTxWithAsset(ctx, client, amount, "USD/2", ledger) +} + +// RunTxWithAsset creates a transaction on the specified ledger with the given amount and asset +func RunTxWithAsset(ctx context.Context, client *client.Formance, amount *big.Int, asset string, ledger string) error { orderID := fmt.Sprint(int64(math.Abs(float64(random.GetRandom())))) dest := fmt.Sprintf("orders:%s", orderID) _, err := client.Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ V2PostTransaction: components.V2PostTransaction{ Postings: []components.V2Posting{{ Amount: amount, - Asset: "USD/2", + Asset: asset, Destination: dest, Source: "world", }}, }, Ledger: ledger, }) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/antithesis.ymlis excluded by!**/*.ymltest/antithesis/workload/go.modis excluded by!**/*.modtest/antithesis/workload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
test/antithesis/workload/bin/cmds/first_default_ledger/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)test/antithesis/workload/internal/utils.go(1 hunks)tools/generator/cmd/root.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/generator/cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/antithesis/workload/bin/cmds/first_default_ledger/main.go (1)
test/antithesis/workload/internal/utils.go (3)
NewClient(41-57)CreateLedger(59-65)Details(20-20)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (4)
test/antithesis/workload/internal/utils.go (2)
NewClient(41-57)ListLedgers(67-79)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)
test/antithesis/workload/bin/cmds/parallel_drive_ledger_create/main.go (1)
test/antithesis/workload/internal/utils.go (4)
NewClient(41-57)RandomBigInt(22-27)CreateLedger(59-65)Details(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests
- GitHub Check: Dirty
- GitHub Check: Push Images to Antithesis
| for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { | ||
| assert.Always( | ||
| volumes.Cmp(new(big.Int)) == 0, | ||
| fmt.Sprintf("aggregated volumes for asset %s should be 0", | ||
| asset, | ||
| ), internal.Details{ | ||
| "error": err, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix incorrect error variable in assertion details.
The err variable in the assertion details is from the GetBalancesAggregated call, which would be nil at this point. The details should contain relevant information about the volumes instead.
for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data {
assert.Always(
volumes.Cmp(new(big.Int)) == 0,
fmt.Sprintf("aggregated volumes for asset %s should be 0",
asset,
), internal.Details{
- "error": err,
+ "asset": asset,
+ "volume": volumes.String(),
})
}🤖 Prompt for AI Agents (early access)
In test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go around lines 42 to 50, the assertion details incorrectly include the variable 'err', which is nil here. Replace 'err' with relevant information about the 'volumes' variable to provide meaningful context in the assertion failure details.
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (2)
20-30: Error handling could be more descriptive.The error handling is functional but could benefit from a more specific error message that includes the ledger name.
if err != nil { - assert.Always(err == nil, "ledger should have been created properly", internal.Details{ + assert.Always(err == nil, fmt.Sprintf("Failed to create ledger %s", ledger), internal.Details{ "error": err, + "ledger": ledger, }) return }
32-33: Consider adding more detailed logging.The completion message could include the ledger name to make logs more traceable in a parallel environment.
- log.Println("composer: parallel_driver_ledger_create: done") + log.Printf("composer: parallel_driver_ledger_create: done (created ledger: %s)", ledger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
test/antithesis/Earthfile(1 hunks)test/antithesis/workload/Earthfile(1 hunks)test/antithesis/workload/bin/cmds/first_default_ledger/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go(1 hunks)test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
- test/antithesis/workload/bin/cmds/first_default_ledger/main.go
- test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go
- test/antithesis/Earthfile
- test/antithesis/workload/Earthfile
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (1)
test/antithesis/workload/internal/utils.go (4)
NewClient(41-57)RandomBigInt(22-27)CreateLedger(59-65)Details(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Push Images to Antithesis
🔇 Additional comments (3)
test/antithesis/workload/bin/cmds/parallel_driver_ledger_create/main.go (3)
1-12: Good package structure and imports.The package structure and imports are well-organized. The code correctly imports the necessary libraries for context, formatting, logging, and big integer operations, as well as the Antithesis assertion library and internal utilities.
13-19: Solid initialization with unique ledger naming.The initialization logic is sound:
- Properly sets up a background context
- Initializes a client using the internal utility
- Generates a unique ledger name using a random positive ID
Good use of the
Abs()method to ensure the randomly generated ID is positive before formatting it into a ledger name.
1-34:❓ Verification inconclusive
Add cleanup or documentation for created resources.
The program creates a ledger but doesn't have a mechanism to clean it up. While this might be handled elsewhere, it would be beneficial to either:
- Add a cleanup step, or
- Add documentation explaining why cleanup isn't needed or how it's handled
This helps prevent resource leakage in testing environments.
🏁 Script executed:
#!/bin/bash # Check if there are any existing cleanup mechanisms for ledgers in the codebase echo "Searching for ledger cleanup mechanisms..." rg -i "delete\s+ledger|cleanup\s+ledger|remove\s+ledger" --type goLength of output: 313
Add cleanup or document resource handling
I searched the repo for ledger‐cleanup routines and found only:
- internal/storage/system/migrations.go (“Remove ledger state column”)
- pkg/client/v2.go (
DeleteLedgerMetadata) – only deletes metadata, not the ledger itselfThere’s currently no API call in this test CLI to delete the created ledger. To avoid resource leakage in your test environment, please either:
- Invoke or implement a proper
DeleteLedger(or similar) helper after creation, or- Add documentation explaining why ledger cleanup isn’t needed here (e.g. handled by a global teardown or another process)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
53-61: Fix incorrect error variable in assertion details.The
errvariable in the assertion details is from the GetBalancesAggregated call, which would be nil at this point. The details should contain relevant information about the volumes instead.for asset, volumes := range aggregated.V2AggregateBalancesResponse.Data { assert.Always( volumes.Cmp(new(big.Int)) == 0, fmt.Sprintf("aggregated volumes for asset %s should be 0", asset, ), internal.Details{ - "error": err, + "asset": asset, + "volume": volumes.String(), }) }
🧹 Nitpick comments (2)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (2)
39-51: Logging improvement suggestion.Consider adding more detailed logging before making the API call to help with debugging and monitoring the workflow.
func checkVolumes(ctx context.Context, client *client.Formance, ledger string) { + log.Printf("composer: parallel_driver_volumes: checking volumes for ledger %s", ledger) aggregated, err := client.Ledger.V2.GetBalancesAggregated(ctx, operations.V2GetBalancesAggregatedRequest{ Ledger: ledger, })
17-22: Consider adding a timeout to the context.For better resource management, consider using a context with timeout to prevent indefinite hanging if an operation gets stuck.
func main() { log.Println("composer: parallel_driver_volumes") - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() client := internal.NewClient()Don't forget to add the time import:
import ( "context" "fmt" "log" "math/big" "os" "sync" + "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (4)
test/antithesis/workload/internal/utils.go (3)
NewClient(41-57)ListLedgers(67-79)Details(20-20)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(9-20)pkg/client/models/operations/v2getbalancesaggregated.go (1)
V2GetBalancesAggregatedRequest(11-18)pkg/client/models/components/v2aggregatebalancesresponse.go (1)
V2AggregateBalancesResponse(10-12)
🔇 Additional comments (1)
test/antithesis/workload/bin/cmds/parallel_driver_volumes/main.go (1)
17-37: LGTM: Good concurrency implementation with WaitGroup.The main function correctly implements the WaitGroup pattern for concurrency, ensuring all goroutines complete before the program exits. This addresses the synchronization issue that was mentioned in past reviews.
Signed-off-by: Clément Salaün <salaun.clement@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/storage/driver/driver.go (2)
58-60: Trim comments that restate the code.Several inline comments repeat what the code already expresses (prior reviewer noted this too). Consider removing or tightening to keep the function compact.
Also applies to: 63-63, 108-108, 114-114, 119-119
85-95: Hard-coded assertion always flags “unreachable”; parameterize and fix typo.This reproduces a prior concern: using
falseguarantees the Unreachable path when a bucket is outdated, which isn’t aligned with the TODO. Make it configurable and correct the “wether” typo. Also add coverage for both expected/unexpected cases.- assert.AlwaysOrUnreachable( - // @todo: replace this with a proper flag detailing wether we're + assert.AlwaysOrUnreachable( + // @todo: replace this with a proper flag detailing whether 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, + d.expectOutdatedBuckets, "Bucket is outdated", map[string]any{ "bucket": l.Bucket, }, )Support code outside this hunk (example):
// in type Driver expectOutdatedBuckets bool // option func WithExpectOutdatedBuckets(v bool) Option { return func(d *Driver) { d.expectOutdatedBuckets = v } }Would you like me to add a focused test that stubs Bucket.IsUpToDate to false and asserts the returned error and the assertion path under both flag values?
🧹 Nitpick comments (2)
internal/storage/driver/driver.go (2)
10-10: Decouple Antithesis SDK from production builds.Importing
github.com/antithesishq/antithesis-sdk-go/assertdirectly couples all builds to the SDK. Consider a thin internal wrapper with build tags to no-op outside Antithesis runs.Example:
// internal/assertx/assert_antithesis.go //go:build antithesis package assertx import "github.com/antithesishq/antithesis-sdk-go/assert" func AlwaysOrUnreachable(cond bool, msg string, kv map[string]any) { assert.AlwaysOrUnreachable(cond, msg, kv) } // internal/assertx/assert_stub.go //go:build !antithesis package assertx func AlwaysOrUnreachable(bool, string, map[string]any) {}Then replace calls to
assert.AlwaysOrUnreachablewithassertx.AlwaysOrUnreachable.
63-70: Avoid resolving the same error twice.Resolve once and branch on that value. This also makes the intent crisper.
- if err := systemStore.CreateLedger(ctx, l); err != nil { - // Handle the case where the ledger already exists - if errors.Is(postgres.ResolveError(err), postgres.ErrConstraintsFailed{}) { - return systemstore.ErrLedgerAlreadyExists - } - return postgres.ResolveError(err) - } + if err := systemStore.CreateLedger(ctx, l); err != nil { + rerr := postgres.ResolveError(err) + if errors.Is(rerr, postgres.ErrConstraintsFailed{}) { + return systemstore.ErrLedgerAlreadyExists + } + return rerr + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
.github/workflows/antithesis.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
internal/storage/driver/driver.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ 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 (1)
internal/storage/driver/driver.go (1)
41-55: Good, clear GoDoc for CreateLedger.Accurately documents the transactional flow and failure semantics.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
test/antithesis/workload/Earthfile (1)
19-19: Pin instrumentor version for reproducible builds.Using @latest risks non-determinism and breakage. Pin a known-good version.
-RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest +RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@v0.5.2
🧹 Nitpick comments (10)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (3)
58-65: Consider surfacing richer context on failure and success.
- Include namespace/name/version in error logs.
- Optionally log the observed Generation/ResourceVersion on success.
No diff required; informational.
67-67: Nit: Replace placeholder log with actionable message.E.g., “upgraded Ledger stack0-ledger to version $tag in namespace formance-systems”.
58-58: Parameterize name/namespace.Hardcoding “stack0-ledger” and “formance-systems” makes reuse harder; read from env vars with sane defaults.
test/antithesis/workload/Earthfile (2)
42-42: Reduce image size and CVE surface after apt-get.Clean apt lists in the same layer.
-RUN apt-get update && apt-get install -y curl +RUN apt-get update && apt-get install -y curl \ + && rm -rf /var/lib/apt/lists/*
31-33: Make the loop robust to spaces and non-files.If any file names contain spaces or if bin/cmds has non-entries, the ls-based expansion can misbehave. Prefer Earthly’s globbing:
-FOR file IN $(ls bin/cmds) - RUN go build -o /out/cmds/$file ./bin/cmds/$file +FOR file IN bin/cmds/* + RUN go build -o /out/cmds/$(basename ${file}) ./bin/cmds/$(basename ${file}) ENDtest/antithesis/config/Justfile (1)
17-19: Guard env and use strict mode in push.push: build-manifest - docker build -f Dockerfile.config -t $ANTITHESIS_REPOSITORY/antithesis-config:k8s_test . - docker push $ANTITHESIS_REPOSITORY/antithesis-config:k8s_test + set -euo pipefail + : "${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY is required}" + docker build --pull -f Dockerfile.config -t "$ANTITHESIS_REPOSITORY/antithesis-config:k8s_test" . + docker push "$ANTITHESIS_REPOSITORY/antithesis-config:k8s_test"test/antithesis/Justfile (4)
5-11: Safer JSON construction (optional).If
$ANTITHESIS_SLACK_REPORT_RECIPIENTcan contain quotes or special chars, prefer generating the payload with a JSON tool to avoid malformed JSON.Example without changing external deps (uses Python, commonly available):
payload="$(python3 - <<'PY' import json, os print(json.dumps({ "params": { "custom.duration": "0.1", "antithesis.report.recipients": os.environ["ANTITHESIS_SLACK_REPORT_RECIPIENT"], "antithesis.config_image": "antithesis-config:k8s_test", "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:k8s_test" } })) PY )" curl ... --data "$payload"
9-9: Pin images for reproducibility; avoid latest/test tags in CI-triggered experiments.
workload:latestandantithesis-config:k8s_testare mutable; runs won’t be reproducible.Consider parameterizing and/or digest-pinning, e.g.:
- "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:k8s_test" \ + "antithesis.images": "workload:${LEDGER_LATEST_TAG};docker.io/library/postgres:15-alpine@${POSTGRES_DIGEST};ghcr.io/formancehq/operator:v2.10.1@${OPERATOR_DIGEST};ghcr.io/formancehq/operator-utils:v2.0.14@${OPERATOR_UTILS_DIGEST};ghcr.io/formancehq/gateway:v2.0.24@${GATEWAY_DIGEST};ghcr.io/formancehq/ledger-instrumented:k8s_test@${LEDGER_INSTRUMENTED_DIGEST}" \If digests aren’t practical, at least replace
latest/k8s_testwith immutable versioned tags produced by your pipeline.Also applies to: 21-21
1-11: DRY the two nearly-identical recipes.Reduce duplication and the chance of drift by introducing a parameterized recipe.
Example:
run duration: # shared preconditions/auth here... # build payload using $duration # curl ... run-6min: requirements-build just run 0.1 run-1h: requirements-build just run 1Also applies to: 13-23
25-28: Add env prechecks to avoid surprising no-op pushes.Fail fast if required vars are missing before building/pushing.
requirements-build: - just config/push + : "${ANTITHESIS_REPOSITORY:?ANTITHESIS_REPOSITORY is required}" + : "${LEDGER_LATEST_TAG:?LEDGER_LATEST_TAG is required}" + just config/push # earthly --push ./image+build --ANTITHESIS_REPOSITORY="$ANTITHESIS_REPOSITORY" earthly --push ./workload+build --ANTITHESIS_REPOSITORY="$ANTITHESIS_REPOSITORY" --LEDGER_LATEST_TAG="$LEDGER_LATEST_TAG"Also verify your environment supports
justsubdirectory invocation (just config/push). If not, usejust --justfile ./config/Justfile push.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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.ymlis excluded by!**/*.ymltest/antithesis/config/manifests/ledger.yamlis excluded by!**/*.yamltest/antithesis/config/manifests/stack.yamlis excluded by!**/*.yamltest/antithesis/workload/go.modis excluded by!**/*.modtest/antithesis/workload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
test/antithesis/Justfile(1 hunks)test/antithesis/config/Justfile(1 hunks)test/antithesis/workload/Earthfile(1 hunks)test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go (2)
cmd/root.go (1)
Version(23-23)test/antithesis/workload/internal/utils.go (1)
Details(20-20)
⏰ 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 (1)
test/antithesis/config/Justfile (1)
8-9: Double-check CRD availability vs. Ledger CR usage.If operator CRDs aren’t applied via regions chart, the Ledger manifest may fail. Ensure regions chart version includes the Ledger CRD matching manifests/ledger.yaml.
If uncertain, keep the operator-crds line enabled for safety.
Also applies to: 13-13
| build-manifest: | ||
| 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 | ||
| # helm template operator oci://ghcr.io/formancehq/helm/operator-crds --version v2.8.0 --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.yaml | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden recipes: fail fast and guard required env.
Add strict shell flags and verify LEDGER_PREVIOUS_TAG is set before templating.
build-manifest:
- rm -f -- tmp/resources.yaml
+ 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
# helm template operator oci://ghcr.io/formancehq/helm/operator-crds --version v2.8.0 --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.yaml📝 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-manifest: | |
| 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 | |
| # helm template operator oci://ghcr.io/formancehq/helm/operator-crds --version v2.8.0 --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.yaml | |
| 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 | |
| # helm template operator oci://ghcr.io/formancehq/helm/operator-crds --version v2.8.0 --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.yaml |
🤖 Prompt for AI Agents
In test/antithesis/config/Justfile around lines 1 to 16, the build-manifest
recipe runs multiple shell commands without strict failure handling or checking
required environment variables; update the recipe to enable strict shell flags
(set -euo pipefail) at the start of the recipe’s shell invocation and add an
explicit guard that verifies LEDGER_PREVIOUS_TAG is set (exit with a clear error
if not) before running the yq templating step so the recipe fails fast and does
not proceed with an unset variable.
| curl --fail \ | ||
| --user "formance:$ANTITHESIS_PASSWORD" \ | ||
| -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:k8s_test", \ | ||
| "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:k8s_test" \ | ||
| } \ | ||
| }' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop leaking credentials; switch to netrc, add JSON header, and harden curl.
Passing $ANTITHESIS_PASSWORD via --user exposes it in process listings. Also, set the proper JSON content-type and add basic resiliency.
Apply this diff:
- 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 --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 '{ \📝 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.
| curl --fail \ | |
| --user "formance:$ANTITHESIS_PASSWORD" \ | |
| -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:k8s_test", \ | |
| "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:k8s_test" \ | |
| } \ | |
| }' | |
| # 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 --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:k8s_test", \ | |
| "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:k8s_test" \ | |
| } \ | |
| }' |
🤖 Prompt for AI Agents
In test/antithesis/Justfile around lines 2 to 11, the curl invocation leaks the
ANTITHESIS_PASSWORD via --user, lacks a JSON content-type header, and has no
retry/resilience flags; remove the --user usage and instead rely on netrc by
instructing the environment or CI to create a .netrc entry for the host with the
Formance credentials and pass curl the netrc file option, add a Content-Type:
application/json header, and harden curl by adding --silent --show-error --fail
--location plus retry flags (e.g., --retry and --retry-delay) to improve
resiliency; ensure data remains JSON-encoded and that secret handling is
documented so the .netrc is created with correct permissions before running this
command.
| curl --fail \ | ||
| --user "formance:$ANTITHESIS_PASSWORD" \ | ||
| -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:k8s_test", \ | ||
| "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:k8s_test" \ | ||
| } \ | ||
| }' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the 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 --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 '{ \📝 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.
| curl --fail \ | |
| --user "formance:$ANTITHESIS_PASSWORD" \ | |
| -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:k8s_test", \ | |
| "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:k8s_test" \ | |
| } \ | |
| }' | |
| : "${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 --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:k8s_test", \ | |
| "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:k8s_test" \ | |
| } \ | |
| }' |
🤖 Prompt for AI Agents
In test/antithesis/Justfile around lines 14 to 23, the curl call lacks the same
security and resiliency flags used in run-1h; update the invocation to use a
netrc file for credentials (e.g. --netrc-file "$ANTITHESIS_NETRC" or --netrc as
appropriate), add an explicit JSON content-type header (-H "Content-Type:
application/json"), and harden curl by adding standard options such as --silent
--show-error --location --fail --retry N --retry-delay M --connect-timeout S
--max-time T (choose sensible values) so the request uses the netrc credentials,
sends proper JSON headers, follows redirects, surfaces errors, and
retries/timeouts for resilience.
| import ( | ||
| "context" | ||
| "log" | ||
| "os" | ||
|
|
||
| "github.com/antithesishq/antithesis-sdk-go/assert" | ||
| "github.com/formancehq/ledger/test/antithesis/internal" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/client-go/dynamic" | ||
|
|
||
| "k8s.io/client-go/tools/clientcmd" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add required imports for patching, trimming, and timeouts.
Needed for the Patch and other fixes.
@@
import (
"context"
+ "encoding/json"
"log"
"os"
+ "strings"
+ "time"
@@
"k8s.io/apimachinery/pkg/runtime/schema"
+ "k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/dynamic"
@@
+ "k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
)📝 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.
| import ( | |
| "context" | |
| "log" | |
| "os" | |
| "github.com/antithesishq/antithesis-sdk-go/assert" | |
| "github.com/formancehq/ledger/test/antithesis/internal" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | |
| "k8s.io/apimachinery/pkg/runtime/schema" | |
| "k8s.io/client-go/dynamic" | |
| "k8s.io/client-go/tools/clientcmd" | |
| ) | |
| import ( | |
| "context" | |
| "encoding/json" | |
| "log" | |
| "os" | |
| "strings" | |
| "time" | |
| "github.com/antithesishq/antithesis-sdk-go/assert" | |
| "github.com/formancehq/ledger/test/antithesis/internal" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | |
| "k8s.io/apimachinery/pkg/runtime/schema" | |
| "k8s.io/apimachinery/pkg/types" | |
| "k8s.io/client-go/dynamic" | |
| "k8s.io/client-go/rest" | |
| "k8s.io/client-go/tools/clientcmd" | |
| ) |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go around
lines 3 to 17, the import block is missing packages required for
creating/patching resources, trimming strings, and handling timeouts; add the
necessary imports (k8s.io/apimachinery/pkg/types for PatchType, strings for
trimming, and time for timeouts) to the import list so the subsequent patch
operations, string trimming calls, and timeout logic compile and work as
intended.
| // build dynamic client | ||
| config, err := clientcmd.BuildConfigFromFlags("", "") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| dyn, err := dynamic.NewForConfig(config) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use in-cluster config with local fallback; add timeout context.
This improves reliability in k8s pods while preserving dev ergonomics.
@@
-// build dynamic client
-config, err := clientcmd.BuildConfigFromFlags("", "")
+// build dynamic client (prefer in-cluster)
+config, err := rest.InClusterConfig()
if err != nil {
- panic(err)
+ config, err = clientcmd.BuildConfigFromFlags("", "")
+ if err != nil {
+ panic(err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // build dynamic client | |
| config, err := clientcmd.BuildConfigFromFlags("", "") | |
| if err != nil { | |
| panic(err) | |
| } | |
| dyn, err := dynamic.NewForConfig(config) | |
| if err != nil { | |
| panic(err) | |
| } | |
| // build dynamic client (prefer in-cluster) | |
| config, err := rest.InClusterConfig() | |
| if err != nil { | |
| config, err = clientcmd.BuildConfigFromFlags("", "") | |
| if err != nil { | |
| panic(err) | |
| } | |
| } | |
| dyn, err := dynamic.NewForConfig(config) | |
| if err != nil { | |
| panic(err) | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go around
lines 26-35, replace the unconditional BuildConfigFromFlags with logic that
first attempts rest.InClusterConfig() and falls back to
clientcmd.BuildConfigFromFlags("", kubeconfig) (using KUBECONFIG env or default
~/.kube/config) so the binary works reliably in-cluster and locally; after
obtaining the rest.Config create the dynamic client as before, and introduce a
context.WithTimeout (e.g., 10s) with defer cancel to use for any subsequent API
calls (and pass that context into client calls), ensuring the config/client
creation uses the in-cluster-first fallback and operations use a bounded
timeout.
test/antithesis/workload/bin/cmds/anytime_version_upgrade/main.go
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,51 @@ | |||
| VERSION 0.7 | |||
|
|
|||
| FROM --platform=linux/amd64 golang:1.24.6 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin base images to stable tags (ideally digests).
golang:1.24.6 may not exist; ubuntu:latest drifts and can break builds or introduce CVEs. Pin to a known minor and/or digest.
-FROM --platform=linux/amd64 golang:1.24.6
+FROM --platform=linux/amd64 golang:1.22
@@
- FROM --platform=linux/amd64 ubuntu:latest
+ FROM --platform=linux/amd64 ubuntu:22.04If you have approved versions, use those or image digests.
Also applies to: 38-38
🌐 Web query:
Does the Docker image tag "golang:1.24.6" exist, and what is the latest stable Go minor release?
💡 Result:
Yes — the golang:1.24.6 tag exists (including variants like 1.24.6-alpine). [1][2]
The latest stable minor release is Go 1.25 (released August 12, 2025). [3]
Sources:
- Go release dashboard (go1.24.6 entry). [1]
- Docker golang images (1.24.6-alpine shown in registries/scans). [2]
- Official Go release notes / download (Go 1.25 release, Aug 12, 2025). [3]
Pin base images to immutable digests
golang:1.24.6 does exist, but relying on unpinned tags risks drift. Upgrade to the current minor release (Go 1.25, released August 12, 2025) and reference its digest—for example:
FROM --platform=linux/amd64 golang:1.25@sha256:<digest>Likewise, replace ubuntu:latest with a specific LTS version and its digest, e.g.:
FROM --platform=linux/amd64 ubuntu:22.04@sha256:<digest>This ensures reproducible, stable builds.
🤖 Prompt for AI Agents
In test/antithesis/workload/Earthfile around line 3, the base image is
referenced by an unpinned tag ("golang:1.24.6"), which risks drift; update the
Go base to the current minor release (Go 1.25) and replace the tag with the
immutable image digest (e.g., golang:1.25@sha256:<digest>) and likewise replace
any ubuntu:latest usage with a specific LTS version and its digest (e.g.,
ubuntu:22.04@sha256:<digest>) so builds are reproducible; obtain the correct
sha256 digests from the official image registries and substitute them in the
FROM lines.
| COPY +sources/src /src | ||
|
|
||
| WORKDIR /src/test/antithesis/workload | ||
| COPY --dir ../../../pkg/client+client/client ../../../pkg/client |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify COPY --dir rename semantics; simplify if not required.
The “+client/client” rename is easy to misread and may be unnecessary if the original layout is already pkg/client/client.
If not required, prefer:
-COPY --dir ../../../pkg/client+client/client ../../../pkg/client
+COPY --dir ../../../pkg/client ../../../pkg/clientIf the rename is intentional, add a short comment explaining why.
🌐 Web query:
In Earthly, is "COPY --dir <src>+<newname> <dst>" the correct syntax to rename the root folder during copy?
💡 Result:
No. The correct forms put the artifact/dir as the source and the new name as the destination. For local dirs:
- COPY --dir test newname
For artifact form: - COPY --dir +target/path newname
Earthly's docs show COPY --dir and artifact refs as +target/artifact-path. [1][2]
Sources:
[1] Earthfile COPY docs (options & examples). [2] Importing / artifact reference examples.
Fix COPY --dir syntax
The current line
COPY --dir ../../../pkg/client+client/client ../../../pkg/client
misuses the + operator—Earthly’s syntax is COPY --dir <src> <dest>, with a leading + only on artifact refs. For a local directory, use for example:
COPY --dir ../../../pkg/client pkg/client
If you meant to rename the root folder, use:
COPY --dir ../../../pkg/client client
Or, for an artifact import:
COPY --dir +target/path newname
Add a comment if any ref or rename is non-obvious.
🤖 Prompt for AI Agents
In test/antithesis/workload/Earthfile around line 24, the COPY --dir invocation
incorrectly uses a '+' inside the source path
("../../../pkg/client+client/client"), violating Earthly syntax; fix by changing
the line to use a plain local source and desired destination (for example COPY
--dir ../../../pkg/client pkg/client or COPY --dir ../../..//pkg/client client
if you intended to rename the root), or if you intended to import an artifact
use the leading plus form (COPY --dir +target/path newname); also add a brief
comment explaining any non-obvious ref or rename.
| ARG --required ANTITHESIS_REPOSITORY | ||
| ARG --required LEDGER_LATEST_TAG | ||
|
|
||
| RUN echo ${LEDGER_LATEST_TAG} > /ledger_latest_tag |
There was a problem hiding this comment.
Avoid trailing newline in /ledger_latest_tag.
echo appends “\n”; use printf for exact content.
-RUN echo ${LEDGER_LATEST_TAG} > /ledger_latest_tag
+RUN printf %s "${LEDGER_LATEST_TAG}" > /ledger_latest_tag📝 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 echo ${LEDGER_LATEST_TAG} > /ledger_latest_tag | |
| RUN printf %s "${LEDGER_LATEST_TAG}" > /ledger_latest_tag |
🤖 Prompt for AI Agents
In test/antithesis/workload/Earthfile around line 49, the current RUN uses echo
which appends a trailing newline to /ledger_latest_tag; replace the echo with a
printf invocation that writes the LEDGER_LATEST_TAG value without adding a
newline (use printf with a %s format and the variable, redirecting output to
/ledger_latest_tag) to ensure the file contains the exact tag and no trailing
newline.
|
|
||
| RUN echo ${LEDGER_LATEST_TAG} > /ledger_latest_tag | ||
|
|
||
| SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also push a content-addressable tag (e.g., the version).
latest is mutable; add an immutable tag for traceability.
-SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:latest
+SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:latest
+SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:${LEDGER_LATEST_TAG}📝 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.
| SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:latest | |
| SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:latest | |
| SAVE IMAGE --push --no-manifest-list ${ANTITHESIS_REPOSITORY}/workload:${LEDGER_LATEST_TAG} |
🤖 Prompt for AI Agents
In test/antithesis/workload/Earthfile around line 51, the image is only pushed
with the mutable tag "latest"; add an additional push of an immutable,
content-addressable tag (for example a version or commit SHA) so the image is
traceable. Modify the Earthfile to produce and push a second tag such as
${ANTITHESIS_REPOSITORY}/workload:${ANTITHESIS_VERSION} (or derive from git
commit SHA) alongside the existing latest push — ensure the image is tagged with
that immutable tag and pushed (or saved) as well so both latest and the
content-addressable tag exist in the registry.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/antithesis/image/Earthfile (1)
42-42: Version the image tag instead of pushing only :latestParameterize the tag for traceability (commit SHA, tag, or build number).
-build: +build: + ARG IMAGE_TAG?=latest ... - SAVE IMAGE --push ghcr.io/formancehq/ledger-instrumented:latest + SAVE IMAGE --push ghcr.io/formancehq/ledger-instrumented:${IMAGE_TAG}
🧹 Nitpick comments (3)
test/antithesis/image/Earthfile (3)
24-24: Make build output more deterministicAdd -trimpath and -buildvcs=false; keeps debug info for symbols while removing host paths/VCS metadata.
- RUN go build -race -o ledger + RUN go build -race -trimpath -buildvcs=false -o ledger
29-41: Consider running as non-root in the runtime imageDrop privileges for better isolation.
COPY (+compile/ledger) /bin/ledger COPY (+compile/symbols) /symbols COPY entrypoint.sh /bin/entrypoint.sh - RUN chmod 777 /bin/entrypoint.sh + RUN chmod 0755 /bin/entrypoint.sh \ + && useradd --system --no-create-home --shell /usr/sbin/nologin ledger + USER ledger
7-8: Deduplicate cache directivesYou define go caches globally and again in compile; keep one to reduce noise.
- CACHE --sharing=shared --id go-mod-cache /go/pkg/mod - CACHE --sharing=shared --id go-cache /root/.cache/go-buildAlso applies to: 12-13
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/workflows/antithesis.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
test/antithesis/image/Earthfile(1 hunks)
🔇 Additional comments (3)
test/antithesis/image/Earthfile (3)
19-28: LGTM: instrumentation + artifact handoff look coherentThe instrument → build → symbols flow is consistent with Antithesis usage.
15-15: No changes required
The Earthfile at../../..defines asourcestarget that exports/src.
39-41: Ensureledger servebinds to port 8080
In test/antithesis/image/entrypoint.sh, invokeledger serve --addr :8080to matchEXPOSE 8080, or update the EXPOSE directive to the actual default listen port.
| CACHE --sharing=shared --id go-cache /root/.cache/go-build | ||
|
|
||
| compile: | ||
| FROM --platform=linux/amd64 golang:1.24.6 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm and pin Go toolchain version
1.24.6 may be behind the latest patch/minor. Please confirm current stable and pin accordingly for reproducibility.
🌐 Web query:
What is the latest stable Go release (minor and patch) as of today?
💡 Result:
The latest stable Go release as of today (September 10, 2025) is Go 1.25.1. [1][2]
Sources:
- Official Go VERSION file (go1.25.1). [1]
- Go announcement: "Go 1.25.1 and Go 1.24.7 are released" (Sep 3, 2025). [2]
Pin Go toolchain to latest stable version (1.25.1)
In test/antithesis/image/Earthfile line 11, update:
- FROM --platform=linux/amd64 golang:1.24.6
+ FROM --platform=linux/amd64 golang:1.25.1📝 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 --platform=linux/amd64 golang:1.24.6 | |
| FROM --platform=linux/amd64 golang:1.25.1 |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around line 11, the Docker base image is
pinned to golang:1.24.6; update the FROM line to use the latest stable Go
toolchain by replacing golang:1.24.6 with golang:1.25.1 (preserve the
--platform=linux/amd64 flag if needed) so the image builds with Go 1.25.1.
| CACHE --sharing=shared --id go-cache /root/.cache/go-build | ||
|
|
||
| COPY ../../..+sources/src /src | ||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid @latest for the instrumentor; pin the version
Use an ARG and pin to a known tag/commit to keep builds reproducible.
compile:
+ ARG ANTITHESIS_GO_INSTRUMENTOR_VERSION?=v0.0.0
- 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@${ANTITHESIS_GO_INSTRUMENTOR_VERSION}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around line 16, the RUN uses @latest for
installing the instrumentor which makes builds non-reproducible; add an ARG
(e.g., INSTRUMENTOR_VERSION) with a pinned tag or commit hash (set a sensible
default), then change the RUN line to reference that ARG (e.g.,
github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@${INSTRUMENTOR_VERSION});
commit the Dockerfile/Earthfile change so builds can be reproduced by overriding
the ARG if needed.
| FROM --platform=linux/amd64 ubuntu:latest | ||
| RUN apt-get update -y && apt-get install -y postgresql-client curl | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin runtime base and reduce image size
Avoid ubuntu:latest; pin LTS and clean apt cache. Also prefer no-install-recommends.
- FROM --platform=linux/amd64 ubuntu:latest
- RUN apt-get update -y && apt-get install -y postgresql-client curl
+ FROM --platform=linux/amd64 ubuntu:24.04
+ RUN apt-get update -y \
+ && apt-get install -y --no-install-recommends postgresql-client curl ca-certificates \
+ && rm -rf /var/lib/apt/lists/*📝 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 --platform=linux/amd64 ubuntu:latest | |
| RUN apt-get update -y && apt-get install -y postgresql-client curl | |
| FROM --platform=linux/amd64 ubuntu:24.04 | |
| RUN apt-get update -y \ | |
| && apt-get install -y --no-install-recommends postgresql-client curl ca-certificates \ | |
| && rm -rf /var/lib/apt/lists/* |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around lines 30–32, the Dockerfile uses
ubuntu:latest and installs packages without cleaning caches; change the base to
a pinned LTS (e.g., ubuntu:22.04) and install packages with
--no-install-recommends, combine commands, and remove apt lists to reduce image
size: use FROM --platform=linux/amd64 ubuntu:22.04 and replace the RUN with a
single line that runs apt-get update && apt-get install -y
--no-install-recommends postgresql-client curl && apt-get clean && rm -rf
/var/lib/apt/lists/*.
| COPY (+compile/symbols) /symbols | ||
| COPY entrypoint.sh /bin/entrypoint.sh | ||
|
|
||
| RUN chmod 777 /bin/entrypoint.sh |
There was a problem hiding this comment.
chmod 777 is over-permissive
Use 0755 to avoid granting write to group/others.
- RUN chmod 777 /bin/entrypoint.sh
+ RUN chmod 0755 /bin/entrypoint.sh📝 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 chmod 777 /bin/entrypoint.sh | |
| RUN chmod 0755 /bin/entrypoint.sh |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around line 37, the Dockerfile uses an
over-permissive file mode (chmod 777) on /bin/entrypoint.sh; change that command
to set the file mode to 0755 so the owner retains write access while
group/others get read+execute only, and keep the rest of the line and file
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/antithesis/image/Earthfile (1)
42-42: Version the pushed image tag (avoid :latest)Use a parameterized, immutable tag for traceability (commit SHA, tag, or timestamp). This was raised earlier; repeating here for this file.
Apply:
- SAVE IMAGE --push ghcr.io/formancehq/ledger-instrumented:latest + ARG IMAGE_TAG?=$(git rev-parse --short HEAD) + SAVE IMAGE --push --no-manifest-list ghcr.io/formancehq/ledger-instrumented:${IMAGE_TAG}
🧹 Nitpick comments (2)
test/antithesis/image/Earthfile (2)
7-8: Deduplicate cache declarationsCaches are declared both at top-level and inside the compile target. Keep the per-target ones and remove the top-level to reduce noise.
Apply:
-CACHE --sharing=shared --id go-mod-cache /go/pkg/mod -CACHE --sharing=shared --id go-cache /root/.cache/go-buildAlso applies to: 12-13
39-40: Add a container healthcheckExpose a simple readiness probe so orchestrators can detect unhealthy containers.
Apply:
ENTRYPOINT ["/bin/entrypoint.sh"] EXPOSE 8080 + HEALTHCHECK --interval=15s --timeout=3s --retries=5 CMD curl -fsS http://127.0.0.1:8080/_health || exit 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/workflows/antithesis.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
test/antithesis/image/Earthfile(1 hunks)
🔇 Additional comments (2)
test/antithesis/image/Earthfile (2)
11-11: Good upgrade: Go toolchain pinned to 1.24.6Pinning to a concrete patch release fixes the previous concern about using a non-existent/unstable tag. LGTM.
24-25: Builder/runner libc alignment with -race-race binaries depend on glibc. Mixing a Debian-based builder with an Ubuntu runtime is usually fine if the runtime glibc is newer, but it can bite in some environments. Consider aligning distro families or explicitly pinning both to known-compatible images.
Would you like me to check the current glibc versions for your selected tags and confirm compatibility?
Also applies to: 30-30
| CACHE --sharing=shared --id go-cache /root/.cache/go-build | ||
|
|
||
| COPY ../../..+sources/src /src | ||
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the instrumentor version for reproducible builds
Using @latest makes builds non-deterministic and can break unexpectedly. Pin a tag/commit and expose it as an ARG.
Apply:
compile:
FROM --platform=linux/amd64 golang:1.24.6
@@
- RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@latest
+ ARG INSTRUMENTOR_VERSION=v0.0.0 # set to known-good tag/commit
+ RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@${INSTRUMENTOR_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 | |
| compile: | |
| FROM --platform=linux/amd64 golang:1.24.6 | |
| ARG INSTRUMENTOR_VERSION=v0.0.0 # set to known-good tag/commit | |
| RUN go install github.com/antithesishq/antithesis-sdk-go/tools/antithesis-go-instrumentor@${INSTRUMENTOR_VERSION} |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around line 16, the Dockerfile uses "go
install ...@latest" which makes builds non-deterministic; change this to accept
a build ARG for the instrumentor version (e.g., INSTRUMENTOR_VERSION) and
replace @latest with that pinned version (defaulting to a specific tag/commit),
so callers can override the ARG but the image builds reproducibly by default;
update the Earthfile to declare the ARG and use it in the RUN line and document
the new ARG in the file comments or build instructions.
| build: | ||
| FROM --platform=linux/amd64 ubuntu:latest | ||
| RUN apt-get update -y && apt-get install -y postgresql-client curl |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid floating base and improve apt hygiene
Pin the runtime base and harden the apt install to reduce image size and supply CA certs.
Apply:
-build:
- FROM --platform=linux/amd64 ubuntu:latest
- RUN apt-get update -y && apt-get install -y postgresql-client curl
+build:
+ FROM --platform=linux/amd64 ubuntu:24.04
+ RUN apt-get update -y \
+ && apt-get install -y --no-install-recommends postgresql-client curl ca-certificates \
+ && rm -rf /var/lib/apt/lists/*📝 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 | |
| build: | |
| FROM --platform=linux/amd64 ubuntu:24.04 | |
| RUN apt-get update -y \ | |
| && apt-get install -y --no-install-recommends postgresql-client curl ca-certificates \ | |
| && rm -rf /var/lib/apt/lists/* |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around lines 29 to 31, the Dockerfile uses a
floating base image and a basic apt install command; update it to pin a specific
Ubuntu LTS tag (e.g., ubuntu:24.04 or another pinned digest/tag) and harden the
apt invocation by installing ca-certificates and postgresql-client with apt-get
install --no-install-recommends, run apt-get update && apt-get install in one
layer, and then clean up apt caches (apt-get clean and remove
/var/lib/apt/lists/*) to reduce image size and ensure CA certs are present.
| RUN chmod 777 /bin/entrypoint.sh | ||
|
|
||
| ENTRYPOINT ["/bin/entrypoint.sh"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop 0777 and run as non-root
0777 is unnecessary and weakens container security. Make the script executable (0755) and run the image as a non-root user.
Apply:
- RUN chmod 777 /bin/entrypoint.sh
+ RUN chmod 0755 /bin/entrypoint.sh
+ # create unprivileged user
+ RUN useradd -r -u 65532 -g nogroup -d /nonexistent -s /usr/sbin/nologin ledger
+ USER 65532:65532📝 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 chmod 777 /bin/entrypoint.sh | |
| ENTRYPOINT ["/bin/entrypoint.sh"] | |
| RUN chmod 0755 /bin/entrypoint.sh | |
| # create unprivileged user | |
| RUN useradd -r -u 65532 -g nogroup -d /nonexistent -s /usr/sbin/nologin ledger | |
| USER 65532:65532 | |
| ENTRYPOINT ["/bin/entrypoint.sh"] |
🤖 Prompt for AI Agents
In test/antithesis/image/Earthfile around lines 37-39 the Dockerfile currently
sets chmod 777 on /bin/entrypoint.sh and leaves the container running as root;
change the permission to 0755, create a non-root user (e.g., groupadd/useradd or
adduser), chown the entrypoint to that user, and add a USER directive before
ENTRYPOINT so the container runs as that non-root user; ensure the entrypoint
remains executable and any required file ownerships are adjusted for the new
user.
Symbiotic Security scan resultsHi @altitude, we've reviewed your PR and here are our findings:
|
| Id | Severity | Vulnerability | File |
|---|---|---|---|
| V0 |  |
TF-0262 Image user should not be 'root' |
test/antithesis/config/Dockerfile.config (1) |
| V1 | |
TF-0423 Default security context configured |
test/antithesis/config/manifests/workload.yaml (9-20) |
| V2 | |
TF-0378 Root file system is not read-only |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V3 | |
TF-0423 Default security context configured |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V4 | |
TF-0423 Default security context configured |
test/antithesis/config/manifests/postgres.yaml (22-42) |
| V5 | |
TF-0378 Root file system is not read-only |
test/antithesis/config/manifests/postgres.yaml (23-42) |
| V6 | |
TF-0423 Default security context configured |
test/antithesis/config/manifests/postgres.yaml (23-42) |
| V7 |  |
TF-0066 Can elevate its own privileges |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V8 | |
TF-0043 Runs as root user |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V9 | |
TF-0042 Image tag ":latest" used |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V10 | |
TF-0087 All container images must start with the *.azurecr.io domain |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V11 | |
TF-0406 All container images must start with a GCR domain |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V12 | |
TF-0109 All container images must start with an ECR domain |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V13 | |
TF-0123 Seccomp policies disabled |
test/antithesis/config/manifests/workload.yaml (10-20) |
| V14 | |
TF-0066 Can elevate its own privileges |
test/antithesis/config/manifests/postgres.yaml (23-42) |
This table only shows the 15 most important issues. To view all the issues introduced by this PR, use our IDE extension.
💡 Type
/symbot review <vuln-id>in a comment to get more details on a specific vulnerability.
Note: You are seeing this because you or someone else with access to this repository has authorized Symbiotic Security to analyze PRs. If you haven't already, create an account and download our extension to detect, learn and fix in your IDE.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/antithesis/workload/internal/utils.go (1)
94-107: Prefer existing random helper over math/rand for consistencyUse
random.RandomChoice(already used in GetRandomAddress) and drop math/rand.Apply this diff:
func GetRandomLedger(ctx context.Context, client *client.Formance) (string, error) { ledgers, err := ListLedgers(ctx, client) if err != nil { return "", fmt.Errorf("error listing ledgers: %v", err) } if len(ledgers) == 0 { return "", fmt.Errorf("no ledgers found") } - randomIndex := rand.Intn(len(ledgers)) - - return ledgers[randomIndex], nil + return random.RandomChoice(ledgers), nil }Also remove the unused
math/randimport at the top.
📜 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/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 as they are similar to previous changes (2)
- test/antithesis/workload/bin/cmds/parallel_driver_transactions/main.go
- test/antithesis/Justfile
🧰 Additional context used
🧬 Code graph analysis (2)
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/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)
test/antithesis/workload/internal/utils.go (5)
pkg/client/formance.go (3)
Formance(50-56)WithClient(90-94)WithRetryConfig(112-116)test/antithesis/workload/bin/cmds/eventually_correct/main.go (1)
IsServerError(141-143)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)
⏰ 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: Push Images to Antithesis
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (4)
test/antithesis/workload/bin/cmds/eventually_correct/main.go (4)
57-58: Don’t kill the whole process from a goroutineReplace
log.Fatalfwith non-fatal logging and return.Apply this diff:
- 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
61-69: Fix assertion details: include actual volume, not err
erris nil here. Provide useful context.Apply this diff:
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(), + }) }
141-143: Remove duplicate IsServerError; use internal.IsServerErrorThis duplicates internal.IsServerError and isn’t used here.
Apply this diff:
-func IsServerError(httpMeta components.HTTPMetadata) bool { - return httpMeta.Response.StatusCode >= 400 && httpMeta.Response.StatusCode < 600 -}
75-80: Invalid range over a constant; code won’t compileYou can’t
rangeover a number. Use a classic for-loop.Apply this diff:
-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 := uint64(0); i < internal.USER_ACCOUNT_COUNT; i++ { address := fmt.Sprintf("users:%d", i) account, err := client.Ledger.V2.GetAccount(ctx, operations.V2GetAccountRequest{ Ledger: ledger, Address: address, })⛔ Skipped due to learnings
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.
| 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) | ||
| } |
There was a problem hiding this comment.
Guard against nil response before using GetHTTPMeta to avoid panic
aggregated may be nil when err != nil, causing a nil deref on GetHTTPMeta().
Apply this diff:
if err != nil {
- if internal.IsServerError(aggregated.GetHTTPMeta()) {
+ 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.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err)
+ log.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 { | |
| 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.Fatalf("error getting aggregated balances for ledger %s: %s", ledger, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/eventually_correct/main.go around lines 47
to 58, the code calls aggregated.GetHTTPMeta() when err != nil which can panic
if aggregated is nil; change the conditional to first check aggregated != nil
before calling GetHTTPMeta (e.g. if aggregated != nil &&
internal.IsServerError(aggregated.GetHTTPMeta()) { ... }) and otherwise fall
back to the log.Fatalf branch (or include the nil case in the assert details) so
no method is invoked on a nil response.
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.
| assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ | ||
| "ledger": ledger, | ||
| "error": err, | ||
| }) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Invert Sometimes condition and enrich details
You want to assert the client can get the account (err == nil). Also, 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,
+ })📝 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.
| assert.Sometimes(err != nil, "Client can aggregate account balances", internal.Details{ | |
| "ledger": ledger, | |
| "error": err, | |
| }) | |
| if err != nil { | |
| continue | |
| } | |
| assert.Sometimes(err == nil, "Client can get account", internal.Details{ | |
| "ledger": ledger, | |
| "address": address, | |
| "error": err, | |
| }) | |
| if err != nil { | |
| continue | |
| } |
🤖 Prompt for AI Agents
In test/antithesis/workload/bin/cmds/eventually_correct/main.go around lines 81
to 87, the assertion currently checks Sometimes(err != nil) but should assert
the client successfully retrieves the account (err == nil); change the assertion
to assert.Sometimes(err == nil, "Client can get account", internal.Details{
"ledger": ledger, "address": addr, "error": err }) and keep the existing if err
!= nil { continue } logic so failures are retried; add the address field to the
details for context.
| 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.
No description provided.