-
Notifications
You must be signed in to change notification settings - Fork 7
[KLC-2516] [KLR-13] Nondeterministic contract deployment outcome timeout boundary #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
030ec02
e3058f7
b442877
330d428
d1f1f35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -472,6 +472,29 @@ func (host *vmHost) handleTimeout(cancelHook context.CancelFunc, done <-chan str | |
| return vmhost.ErrExecutionFailedWithTimeout | ||
| } | ||
|
|
||
| // waitExecutionWithDeterministicCompletion waits for execution completion or timeout. | ||
| // If both completion and timeout are observable at the same boundary, completion wins: | ||
| // a closed done channel proves execution finished within budget, while an expired | ||
| // context also reflects how long this goroutine waited to be scheduled. | ||
| func (host *vmHost) waitExecutionWithDeterministicCompletion( | ||
| ctx context.Context, | ||
| cancelHook context.CancelFunc, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this isn't a hook cancel. Both call sites pass the full cleanup closure from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, it is the full cleanup closure today. Leaving it as a nit though — the name goes back to being accurate once the teardown issue is sorted out separately. |
||
| done <-chan struct{}, | ||
| ) error { | ||
| select { | ||
| case <-done: | ||
| return nil | ||
| case <-ctx.Done(): | ||
| // Deterministic tie-breaker: if execution also completed, completion wins. | ||
| select { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this inner select is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That held for timeout-wins, but the flip inverted it — the inner check is on done now, and a channel has no |
||
| case <-done: | ||
| return nil | ||
| default: | ||
| return host.handleTimeout(cancelHook, done) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
RomuloSiebra marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // RunSmartContractCreate executes the deployment of a new contract | ||
| func (host *vmHost) RunSmartContractCreate(input *vmcommon.ContractCreateInput) (vmOutput *vmcommon.VMOutput, err error) { | ||
| err = validateVMInput(&input.VMInput) | ||
|
|
@@ -534,12 +557,8 @@ func (host *vmHost) RunSmartContractCreate(input *vmcommon.ContractCreateInput) | |
| host.logFromGasTracer("init") | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| // Normal termination | ||
| return | ||
| case <-ctx.Done(): | ||
| err = host.handleTimeout(cancel, done) | ||
| if timeoutErr := host.waitExecutionWithDeterministicCompletion(ctx, cancel, done); timeoutErr != nil { | ||
| err = timeoutErr | ||
| } | ||
|
|
||
| return | ||
|
|
@@ -618,11 +637,8 @@ func (host *vmHost) RunSmartContractCall(input *vmcommon.ContractCallInput) (vmO | |
| host.logFromGasTracer(input.Function) | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| // Normal termination. | ||
| case <-ctx.Done(): | ||
| err = host.handleTimeout(cancel, done) | ||
| if timeoutErr := host.waitExecutionWithDeterministicCompletion(ctx, cancel, done); timeoutErr != nil { | ||
| err = timeoutErr | ||
| } | ||
|
|
||
| return vmOutput, err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package hostCore | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| contextmock "github.com/klever-io/klever-go/kvm/mock/context" | ||
| "github.com/klever-io/klever-go/kvm/vmhost" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestVmHost_WaitExecutionWithDeterministicCompletion_DoneOnly(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| runtimeCtx := &contextmock.RuntimeContextMock{} | ||
| host := &vmHost{runtimeContext: runtimeCtx} | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| hookCtx, cancelHook := context.WithCancel(context.Background()) | ||
| done := make(chan struct{}) | ||
| close(done) | ||
|
|
||
| err := host.waitExecutionWithDeterministicCompletion(ctx, cancelHook, done) | ||
| require.NoError(t, err) | ||
| require.Nil(t, runtimeCtx.FailExecutionErr) | ||
|
|
||
| select { | ||
| case <-hookCtx.Done(): | ||
| t.Fatalf("hook context should not be canceled on normal completion") | ||
| default: | ||
| } | ||
| } | ||
|
|
||
| func TestVmHost_WaitExecutionWithDeterministicCompletion_TimeoutWhileExecutionInFlight(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| timeoutStarted := make(chan struct{}) | ||
| runtimeCtx := &contextmock.RuntimeContextMock{ | ||
| FailExecutionCalled: func(error) { close(timeoutStarted) }, | ||
| } | ||
| host := &vmHost{runtimeContext: runtimeCtx} | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| hookCtx, cancelHook := context.WithCancel(context.Background()) | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| <-timeoutStarted | ||
| close(done) | ||
| }() | ||
|
RomuloSiebra marked this conversation as resolved.
|
||
|
|
||
| err := host.waitExecutionWithDeterministicCompletion(ctx, cancelHook, done) | ||
| require.Equal(t, vmhost.ErrExecutionFailedWithTimeout, err) | ||
| require.Equal(t, vmhost.ErrExecutionFailedWithTimeout, runtimeCtx.FailExecutionErr) | ||
|
|
||
| select { | ||
| case <-done: | ||
| default: | ||
| t.Fatalf("timeout path should wait for execution to finish before returning") | ||
| } | ||
|
|
||
| select { | ||
| case <-hookCtx.Done(): | ||
| default: | ||
| t.Fatalf("hook context should be canceled on timeout") | ||
| } | ||
| } | ||
|
|
||
| func TestVmHost_WaitExecutionWithDeterministicCompletion_TieBreakerCompletionWins(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| for i := 0; i < 100; i++ { | ||
| runtimeCtx := &contextmock.RuntimeContextMock{} | ||
| host := &vmHost{runtimeContext: runtimeCtx} | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| hookCtx, cancelHook := context.WithCancel(context.Background()) | ||
| done := make(chan struct{}) | ||
| close(done) | ||
|
|
||
| err := host.waitExecutionWithDeterministicCompletion(ctx, cancelHook, done) | ||
| require.NoError(t, err) | ||
| require.Nil(t, runtimeCtx.FailExecutionErr) | ||
|
|
||
| select { | ||
| case <-hookCtx.Done(): | ||
| t.Fatalf("hook context should not be canceled when completion wins the tie") | ||
| default: | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: klever-io/klever-go
Length of output: 11334
🏁 Script executed:
Repository: klever-io/klever-go
Length of output: 19595
🌐 Web query:
Go specification composite literals unkeyed struct literal external package adding field compatibility💡 Result:
In the Go programming language, using unkeyed struct composite literals for types defined in an external package is strongly discouraged and considered a compatibility risk [1][2][3]. While the Go language specification permits unkeyed composite literals [4], adding a field to a struct type defined in an external package will break any existing code in other packages that initializes that struct using an unkeyed literal [1][3]. Because the unkeyed syntax relies on the specific order and number of fields, adding a new field changes the expected structure, causing compilation errors in consuming code [5][3]. To maintain backward compatibility when an API evolves, it is a standard Go practice to use keyed composite literals [1][3]. Keyed literals specify the field names (e.g., T{Field: value}), which decouples the initialization from the exact field order and number [3]. This ensures that adding a new field—which will simply default to its zero value if omitted in the literal—does not break existing client code [5][3]. The Go toolchain includes the 'go vet' command (specifically the 'composite' pass), which is designed to identify and warn against the use of unkeyed composite literals for types defined in external packages [2][6][7]. The tool exempts locally defined types from this check because developers can simultaneously update both the struct definition and its usages within the same package, mitigating the risk of breaking changes [2][7].
Citations:
Preserve
RuntimeContextMockpositional compatibility.RuntimeContextMockis exported. AddingFailExecutionCalledmakes downstream unkeyed literals fail to compile. Keep this hook outside the exported struct or use a compatible API.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every RuntimeContextMock literal in the repo is keyed, and the module builds and tests clean with the field added — an unkeyed one would have failed to compile. Keeping it, since the Called-hook pattern is what the other mocks here use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomuloSiebra, keyed literals and successful local tests confirm that this change does not break in-repository callers.The compatibility concern applies to external consumers of the exported
RuntimeContextMocktype. An external unkeyed literal will fail to compile after this field is added. The existingCalled-hook convention supports the pattern, but it does not remove that public API risk.I will keep this finding open unless
kvm/mock/contextis explicitly exempt from backward-compatibility requirements.✏️ Learnings added
You are interacting with an AI system.