Cleanups / notes

Author: dandavison

chasm/engine.go

@@ -175,6 +175,9 @@ func UpdateWithNewEntity[C Component, I any, O1 any, O2 any](
 //   - consider remove ComponentRef from the return value and allow components to get
 //     the ref in the transition function. There are some caveats there, check the
 //     comment of the NewRef method in MutableContext.
+//
+// UpdateComponent applies updateFn to the component identified by the supplied component reference.
+// It returns the result, along with the new component reference.
 func UpdateComponent[C Component, R []byte | ComponentRef, I any, O any](
 	ctx context.Context,
 	r R,
@@ -206,6 +209,8 @@ func UpdateComponent[C Component, R []byte | ComponentRef, I any, O any](
 	return output, newSerializedRef, err
 }
 
+// ReadComponent returns the result of evaluating readFn against the current state of the component
+// identified by the supplied component reference.
 func ReadComponent[C Component, R []byte | ComponentRef, I any, O any](
 	ctx context.Context,
 	r R,

chasm/lib/activity/handler.go

@@ -104,9 +104,9 @@ func pollActivityExecutionWaitAnyStateChange(
 	req *activitypb.PollActivityExecutionRequest,
 ) (*activitypb.PollActivityExecutionResponse, []byte, error) {
 
-	// TODO(dan): it is not guaranteed that the response data will differ from that received on a
-	// previous call. However, this is confusing API behavior: we don't want the server to say
-	// "there's been a change" while returning data in which the change is not apparent.
+	// TODO(dan): do we want to guarantee that response data will differ from that received when the
+	// token was obtained? It's potentially confusing for the server to say "there's been a change"
+	// while returning data in which the change is not apparent.
 
 	refBytesFromToken := req.GetFrontendRequest().
 		GetWaitPolicy().(*workflowservice.PollActivityExecutionRequest_WaitAnyStateChange).
@@ -161,17 +161,14 @@ func pollActivityExecutionWaitAnyStateChange(
 				// The runID from the token doesn't match this shard's state. We return immediately,
 				// on the basis that this constitutes a state change. If the runID from the token is
 				// ahead of this shard's state then this will be detected by shard ownership or
-				// staleness checks and the caller will receive an error. Therefore we can assume
-				// that the runID from the token is behind the shard state and that it's appropriate
-				// to report a state change to the caller.
+				// staleness checks and the caller will receive an error.
 				response, err := a.buildPollActivityExecutionResponse(ctx, req)
 				if err != nil {
 					return nil, true, err
 				}
 				return response, true, nil
 			}
 
-			// TODO(dan): is this leaking too much detail about VTs?
 			refComparison, err := chasm.CompareComponentRefs(&lastSeenRef, &currentRef)
 			if err != nil {
 				return nil, false, err
@@ -204,7 +201,7 @@ func pollActivityExecutionWaitCompletion(
 	ctx context.Context,
 	req *activitypb.PollActivityExecutionRequest,
 ) (*activitypb.PollActivityExecutionResponse, []byte, error) {
-	// TODO(dan): untested
+	// TODO(dan): implement functional test when RecordActivityTaskCompleted is implemented
 	return chasm.PollComponent(
 		ctx,
 		chasm.NewComponentRef[*Activity](chasm.EntityKey{

chasm/ref.go

@@ -157,12 +157,12 @@ func ProtoRefToComponentRef(pRef *persistencespb.ChasmComponentRef) ComponentRef
 	}
 }
 
+// TODO(dan): is this leaking too much detail about VTs?
+//
 // Compare compares the entity versioned transition of two ComponentRefs. Returns -1 if a < b, 0 if
 // a == b, 1 if a > b, where a and b are compared according to their versioned transitions using
-// transitionhistory.Compare. Note that this plies that a component ref without a versioned
+// transitionhistory.Compare. Note that this implies that a component ref without a versioned
 // transition compares less than any component ref with a versioned transition.
-//
-// TODO(dan): is this leaking too much detail about VTs?
 func CompareComponentRefs(a, b *ComponentRef) (int, error) {
 	if a.EntityKey != b.EntityKey {
 		return 0, serviceerror.NewInvalidArgument("component refs have different entity keys and cannot be compared")

chasm/tree.go

@@ -2306,7 +2306,6 @@ func (n *Node) IsStateDirty() bool {
 func (n *Node) IsStale(
 	ref ComponentRef,
 ) error {
-
 	// The point of this method to access the private entityLastUpdateVT field in componentRef,
 	// and avoid exposing it in the public CHASM interface.
 	if ref.entityLastUpdateVT == nil {

service/history/api/get_workflow_util.go

@@ -69,7 +69,6 @@ func GetOrPollMutableState(
 	if err != nil {
 		return nil, err
 	}
-
 	currentVersionHistory, err := versionhistory.GetCurrentVersionHistory(response.GetVersionHistories())
 	if err != nil {
 		return nil, err
@@ -312,9 +311,6 @@ func GetMutableStateWithConsistencyCheck(
 				return transitionhistory.StalenessCheck(transitionHistory, versionedTransition) == nil
 			}
 
-			// TODO(dan): the line above implies that, given transitionhistory.StalenessCheck(), all
-			// the version history checks below are unnecessary. Is this true?
-
 			currentVersionHistory, err := versionhistory.GetCurrentVersionHistory(mutableState.GetExecutionInfo().GetVersionHistories())
 			if err != nil {
 				return false

service/history/chasm_engine.go

@@ -100,10 +100,12 @@ func (e *ChasmEngine) SetShardController(
 	e.shardController = shardController
 }
 
+// Start starts the ChasmEngine
 func (e *ChasmEngine) Start() {
 	e.notifier.Start()
 }
 
+// Stop stops the ChasmEngine
 func (e *ChasmEngine) Stop() {
 	e.notifier.Stop()
 }
@@ -292,7 +294,7 @@ func (e *ChasmEngine) PollComponent(
 ) ([]byte, error) {
 	// 1. Acquire lock
 	// 2. Error if shard entity VT < requestRef VT ('stale state')
-	// 3. Release lock and return if predicate satisfied
+	// 3. If predicate satisfied, release lock and return
 	// 4. Subscribe to notifications for this entity
 	// 5. Release lock
 	// 6. On notification repeat (1) and (2)
@@ -376,7 +378,7 @@ func (e *ChasmEngine) PollComponent(
 				return newRef, nil
 			}
 		case <-longPollCtx.Done():
-			// TODO(dan): or return empty response?
+			// TODO(dan): return empty response?
 			return nil, serviceerror.NewDeadlineExceeded("long-poll timed out")
 		}
 	}

tests/standalone_activity_test.go

@@ -146,7 +146,7 @@ func (s *standaloneActivityTestSuite) Test_PollActivityExecution_WaitAnyStateCha
 		})
 	}()
 
-	// TODO(dan): race here: subscription might not be established yet?
+	// TODO(dan): race here: subscription might not be established yet
 
 	// Worker picks up activity task, triggering transition (via RecordActivityTaskStarted)
 	go func() {
@@ -194,6 +194,11 @@ func (s *standaloneActivityTestSuite) Test_PollActivityExecution_WaitAnyStateCha
 	require.ErrorContains(t, err, "cached mutable state could potentially be stale")
 }
 
+func (s *standaloneActivityTestSuite) Test_PollActivityExecution_WaitCompletion() {
+	t := s.T()
+	t.Skip("TODO(dan): implement test when RecordActivityTaskCompleted is implemented")
+}
+
 func (s *standaloneActivityTestSuite) startActivity(ctx context.Context, activityID string, taskQueue string) (*workflowservice.StartActivityExecutionResponse, error) {
 	return s.FrontendClient().StartActivityExecution(ctx, &workflowservice.StartActivityExecutionRequest{
 		Namespace:  s.Namespace().String(),