Fix hashing step ids in loops #72
Conversation
albertchae
force-pushed
the
albert/INN-3329-hash-steps-loop
branch
from
10f39be to
b4b2f5b
Compare
| while (true) { | ||
| possibleStepId = "$id:$stepNumber" | ||
| if (possibleStepId !in stepIds) { | ||
| break | ||
| } | ||
| stepNumber++ | ||
| } |
There was a problem hiding this comment.
not sure if there's a better way to find the next available stepNumber but looks like this hasn't been a problem in the inngest-js SDK yet so leaving it as is for now
There was a problem hiding this comment.
the Go SDK does something like this.
https://github.com/inngest/inngestgo/blob/0a00daba0b2db68ff0f080f787cf63f0a63b44d8/internal/sdkrequest/manager.go#L123-L131
There was a problem hiding this comment.
doing a loop every time to find the next possible one might not be the most efficient. probably having a map or something of the kind to track state temporarily as it goes through the existing state would be ideal
There was a problem hiding this comment.
cool, that's the algorithm I was envisioning anyway. updated to store count in a map
| int runningCount = 10; | ||
| for (int i = 0; i < 5; i++) { | ||
| int effectivelyFinalVariableForLambda = runningCount; | ||
| runningCount = step.run("add-ten", () -> effectivelyFinalVariableForLambda + 10, Integer.class); |
There was a problem hiding this comment.
this would not compile if I tried to use runningCount directly in the lambda
| RunEntry<Object> loopRun = devServer.runsByEvent(loopEvent).first(); | ||
| assertEquals("Completed", loopRun.getStatus()); | ||
|
|
||
| assertEquals(60, loopRun.getOutput()); |
There was a problem hiding this comment.
was returning 20 before fix to the SDK
albertchae
requested review from
djfarrelly,
darwin67,
jpwilliams and
tonyhb
as code owners
|
|
||
| // use the seen count so far for current step, increment for next time | ||
| val stepNumber = stepIdsToSeenCount[id] | ||
| stepIdsToSeenCount[id] = stepIdsToSeenCount.getValue(id) + 1 |
There was a problem hiding this comment.
stepIdsToSeenCount[id]++ does not compile because the return value on [] map access is T?, even with the !in check above 😭
https://blog.danlew.net/2017/06/14/convincing-the-kotlin-compiler-that-code-is-safe/
albertchae
force-pushed
the
albert/INN-3329-hash-steps-loop
branch
from
f8e69f7 to
7bf72a1
Compare
| val stepNumber = stepIdsToSeenCount[id] | ||
| stepIdsToSeenCount[id] = stepIdsToSeenCount.getValue(id) + 1 | ||
|
|
||
| return "$id:$stepNumber" |
There was a problem hiding this comment.
We may also need a check here to see is this step ID already exists.
A user could do something like:
- Run
"my-step" - Run
"my-step:1"(user explicitly specifying this string) - Run a loop of
"my-step"steps
I think here this would result in us redeclaring "my-step:1" in the first iteration of the loop even though we'd like to skip it and go straight to "my-step:2".
These IDs just being strings means a user can accidentally stumble into our [ID]:[count] format and break some stuff. 😄
There was a problem hiding this comment.
OK pushed a commit to combine both a hash for O(1) in most cases and a loop afterwards just in case a user used that stepId already. Correct me if I'm wrong but is this a bug in the Go SDK then https://github.com/inngest/inngestgo/blob/0a00daba0b2db68ff0f080f787cf63f0a63b44d8/internal/sdkrequest/manager.go#L123-L131 @darwin67 ?
This seems like it could potentially be worth reserving some delimiter characters for metadata if Inngest has other cases where it would want to modify the user provided stepId.
There was a problem hiding this comment.
That works! Thank you.
Mm there's definitely some silly edge case that would be unlikely to hit where users are utilizing *:n step IDs explicitly, but that exists everywhere.
Looks like that'd be a bug in Go too, aye. Long-term we can start to shift this over to something safer; it'd be great to not be directly influencing the ID internally for the hash, but requires a versioned change across SDKs.
There was a problem hiding this comment.
I thought of another edge case where the user could reuse a my-step:n name after a loop that used it, so I updated the test and logic to handle that too
Per https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#512-ids-and-hashing, add `:n` starting with `:1` for repeated instances of a step id Mostly similar to inngest-js implementation https://github.com/inngest/inngest-js/blob/79069e1a3d700624ce49b323922c113fc952bcc6/packages/inngest/src/components/execution/v1.ts#L819-L831 The inngest-js SDK currently optionally warns of parallel indexing, but this isn't in scope for beta so I left it out
The hash means we don't have to loop from 0 every time and for most cases will just correctly return us the next unused stepId, but looping afterwards guarantees we don't collide with a user defined stepId. So this will be O(1) in most cases and potentially O(n) for pathological functions that have many steps manually named with the `:n` suffix
albertchae
force-pushed
the
albert/INN-3329-hash-steps-loop
branch
from
7bf72a1 to
43373c8
Compare
| val stepNumber = stepIdsToSeenCount[id] | ||
| stepIdsToSeenCount[id] = stepIdsToSeenCount.getValue(id) + 1 | ||
|
|
||
| return "$id:$stepNumber" |
There was a problem hiding this comment.
That works! Thank you.
Mm there's definitely some silly edge case that would be unlikely to hit where users are utilizing *:n step IDs explicitly, but that exists everywhere.
Looks like that'd be a bug in Go too, aye. Long-term we can start to shift this over to something safer; it'd be great to not be directly influencing the ID internally for the hash, but requires a versioned change across SDKs.
…already allocated in loop
Summary
Per https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#512-ids-and-hashing,
add
:nstarting with:1for repeated instances of a step idRefactored to be a combination of golang and inngest-js implementation to handle edge case of user defined stepId colliding
The inngest-js SDK currently optionally warns of parallel indexing, but
this isn't in scope for beta so I left it out.
Checklist
Update documentationN/A documented by sdk specRelated