test: fix race condition in CreateSpaceRequest #1236
Conversation
WalkthroughCreateSpaceRequest stopped waiting for a provisioned namespace via Host().WaitForSpace and now uses the Space returned by the signup flow as parentSpace. Separately, VerifySpaceRelatedResources now adds an explicit WaitForSpace refresh to ensure the Space has the expected tier, Provisioned condition, and at least one provisioned namespace. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-01T09:51:30.271ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
testsupport/space/spacerequest.go
Outdated
| Execute(t) | ||
|
|
||
| // wait for the namespace to be provisioned since we will be creating the spacerequest into it. | ||
| parentSpace, err := awaitilities.Host().WaitForSpace(t, user.Space.Name, wait.UntilSpaceHasAnyProvisionedNamespaces()) | ||
| parentSpace, err := awaitilities.Host().WaitForSpace(t, user.Space.Name, | ||
| wait.UntilSpaceHasTier("appstudio"), | ||
| wait.UntilSpaceHasConditions(wait.Provisioned()), | ||
| wait.UntilSpaceHasAnyProvisionedNamespaces()) |
There was a problem hiding this comment.
looking at how the Signup is created:
- it defines the needed & expected space tier as
appstudio - and it also calls
EnsureMURwhich should trigger the evaluation of the provisioned namespaces:
toolchain-e2e/testsupport/signup_request.go
Lines 303 to 313 in b1147e8
so when the execution of the "SignupRequest" logic is finished, then the space should be already in the expected state, so I'm wondering how the race condition could happen. Or I'm maybe missing some small and important detail 🙂
There was a problem hiding this comment.
Thanks! I did not know!
The problem is by the time we call GetDefaultNamespace(parentSpace.Status.ProvisionedNamespaces)) it still has the -dev one and not the -tenant
There was a problem hiding this comment.
There was a problem hiding this comment.
ok, the test execution probably simply happens too quickly. I would rather update the common logic in VerifySpaceRelatedResources:
toolchain-e2e/testsupport/user_assertions.go
Lines 230 to 274 in b1147e8
so just move this code that you added in your PR to the end of the function mentioned above - something like this:
...
space, err = awaitilities.Host().WaitForSpace(t, space.Name,
wait.UntilSpaceHasTier(spaceTierName),
wait.UntilSpaceHasConditions(wait.Provisioned()),
wait.UntilSpaceHasAnyProvisionedNamespaces())
require.NoError(t, err)
return space
}WDYT?
There was a problem hiding this comment.
Sounds good! Addressed, thanks!
| parentSpace := user.Space | ||
|
|
||
| // create the space request in the "default" namespace provisioned by the parentSpace | ||
| spaceRequest := NewSpaceRequest(t, append(opts, WithNamespace(GetDefaultNamespace(parentSpace.Status.ProvisionedNamespaces)))...) |
There was a problem hiding this comment.
I'm afraid that now parentSpace.Status.ProvisionedNamespaces might be empty, when this line is executed, thus the spacerequest will not be created. But maybe I'm wrong.
There was a problem hiding this comment.
it should not - look at the updated code in the other file/function - it has condition wait.UntilSpaceHasAnyProvisionedNamespaces() which ensures that there is gonna be some provisioned namespace set
There was a problem hiding this comment.
Ok I see now, I've missed that this VerifySpaceRelatedResources was called indirectly by the test.
It looks like we were already waiting for the space https://github.com/mfrancisc/toolchain-e2e/blob/8f4432ee3e06e75b90989469cee0aea29c28ab34/testsupport/user_assertions.go#L243-L249, we were probably only missing that wait.UntilSpaceHasAnyProvisionedNamespaces() in there.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, rsoaresd, xcoulon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
4 participants
Description
In
CreateSpaceRequest(), the parent space is created with theappstudiotier, butGetDefaultNamespace()was called immediately after waiting only forUntilSpaceHasAnyProvisionedNamespaces().This created a race condition where:
base1ns)base1nstier creates a-devnamespaceappstudiotierappstudiotier doesn't have a-devnamespace, so it deletes it and creates a-tenantnamespace insteadAssisted-by: Cursor
Issue ticket number and link
SANDBOX-1502
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.