improvement(container-runtime): Simplify dirty state management in ContainerRuntime #24646

markfields · 2025-05-16T22:07:35Z

Description

Refactors dirty state tracking without making any behavior changes.

Dirty state tracking in ContainerRuntime appeared very ad hoc where the reality is the logic is quite simple:

isDirty is defined as (not attached) OR (has pending messages in Outbox or PendingStateManager)
Any time any of these underlying states changes, we need to check if this results in a change to dirty state, and emit the "dirty"/"saved" event if so

Reviewer Guidance

The sequence of commit was very deliberate, with extended commit descriptions explaining the change and why it's a no-op. Here's the one-line commit titles (read up from the bottom):

fdbd24b (HEAD -> cr/dirty-saved, fork/cr/dirty-saved) Final clean up
e82de84 Consolidate two updateDocumentDirtyState calls
8e78133 Remove redundant if checks before updateDocumentDirtyState
~~f3ec7e5 Update isDirty to compute it, not use last emitted.~~
e675d34 Rename only: dirtyContainer to lastEmittedDirty
3a729f4 Remove the asserts from updateDocumentDirtyState
cc05d47 Remove updateDocumentDirtyState param
03faad4 Update each updateDocumentDirtyState callsite to use currentDirtyState()
8ccd56c Prep

- Introduce currentDirtyState() - Some no-op changes to simplify the code - Some other small changes for this PR that won't affect subsequent

And add temp notes explaining why the old hardcoded values match the computed value

They're provably true now that we're using currentDirtyState rather than taking an arg These asserts give even more confidence to this change

This will only change the result of public isDirty for those moments between when a change to attach or pending state happens and when we call updateDocumentDirtyState. I checked all these codepaths, and they are all synchronous, so this change will be a no-op for callers of this public API

In each case: - The if check is equivalent to "no longer dirty" (meaning, we would skip if dirty) - So the question is*: Could we have just switched from saved to dirty? No. - Processing an op can't make us dirty from saved - Attaching can't make us dirty from saved *This is the case where this commit's change would result in an extra dirty event where there isn't one today.

Copilot

Pull Request Overview

This PR refactors the dirty state management logic in ContainerRuntime without changing behavior.

The test file now uses describe.only (with debug markers) instead of describe.
The dirty state variable is renamed from dirtyContainer to lastEmittedDirty, and updateDocumentDirtyState is refactored to compute the dirty state rather than receiving it as an argument.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/runtime/container-runtime/src/test/containerRuntime.spec.ts	Changed test suite block to use describe.only with added debug comments, potentially limiting overall test execution
packages/runtime/container-runtime/src/containerRuntime.ts	Renamed dirtyContainer to lastEmittedDirty and updated the updateDocumentDirtyState logic to simplify dirty state management

Comments suppressed due to low confidence (2)

packages/runtime/container-runtime/src/test/containerRuntime.spec.ts:851

The added debug markers ('//* ONLY') and the use of 'describe.only' may inadvertently limit the test suite execution. Please remove these debug comments and change 'describe.only' back to 'describe' to ensure that all tests run.

//* ONLY

packages/runtime/container-runtime/src/containerRuntime.ts:3313

Ensure that 'UsageError' is properly imported or defined in this file to avoid potential runtime issues.

throw new UsageError("already in staging mode");

packages/runtime/container-runtime/src/containerRuntime.ts

markfields · 2025-05-16T23:29:07Z

Oops - I missed the point of isContainerMessageDirtyable, which is that some messages will sit in PSM but not make the container dirty. Thankfully a bunch of tests are failing.

Couple different ideas for how to fix it:

Add an override flag that indicates that the only pending messages are not "dirtyable"
Flip isDirty back to returning the class field (rename lastEmittedDirty back to something else). Leave the rest of the change where update fn doesn't take a param, etc.
Actually inspect the messages in Outbox/PSM inside isDirty call. This is the most correct, and I think even fixes bugs in the current code (where other calls to update don't consider non-dirtyable messages). Shouldn't be costly perf-wise since in most cases the first message we check is dirtyable

The more correct fix is to compute dirty state based on isDirtyable logic every time

This reverts commit 44ea39f.

anthony-murphy · 2025-05-19T15:14:40Z

Oops - I missed the point of isContainerMessageDirtyable, which is that some messages will sit in PSM but not make the container dirty. Thankfully a bunch of tests are failing.

Couple different ideas for how to fix it:

Add an override flag that indicates that the only pending messages are not "dirtyable"

Flip isDirty back to returning the class field (rename lastEmittedDirty back to something else). Leave the rest of the change where update fn doesn't take a param, etc.

Actually inspect the messages in Outbox/PSM inside isDirty call. This is the most correct, and I think even fixes bugs in the current code (where other calls to update don't consider non-dirtyable messages). Shouldn't be costly perf-wise since in most cases the first message we check is dirtyable

why do we not dirty the container on some ops? Does this prevent switching from read to write connection mode or something?

It seems tests still fail

Right before updating isDirty to calculate

markfields · 2025-05-20T16:33:22Z

Back to Draft state. Might actually abandon, trying 1 or two more things to keep the current behavior but make it slightly more clear/consistent

packages/runtime/container-runtime/src/containerRuntime.ts

markfields · 2025-05-20T18:38:27Z

packages/runtime/container-runtime/src/containerRuntime.ts

-						if (this.dirtyContainer !== checkpointDirtyState) {
-							this.updateDocumentDirtyState(checkpointDirtyState);
-						}
+						this.updateDocumentDirtyState();


This is another place where we recompute rather than restoring the previous last-emitted. Won't affect summarizer (we don't use Order Sequentially there), so should be ok.

Fixed - Updated the "dirty" computation to account for these all the time.

markfields · 2025-05-21T04:00:47Z

Test failures are due to a bug with submitting ID Allocation op before replaying pending states but we don't schedule a flush. So this PR makes the container dirty if that ID Allocation op is the only op, but then it can get stuck that way.

This prompted me to revive #24545 which delays the submission of that op until we're submitting other ops, which solves this issue. Blocking this PR until that one goes in.

It shouldn't be necessary anymore since the dirty state will be correct since we're computing it robustly.

markfields added 9 commits May 16, 2025 19:39

Prep

8ccd56c

- Introduce currentDirtyState() - Some no-op changes to simplify the code - Some other small changes for this PR that won't affect subsequent

Update each updateDocumentDirtyState callsite to use currentDirtyState()

03faad4

And add temp notes explaining why the old hardcoded values match the computed value

Remove updateDocumentDirtyState param

cc05d47

Remove the asserts from updateDocumentDirtyState

3a729f4

They're provably true now that we're using currentDirtyState rather than taking an arg These asserts give even more confidence to this change

Rename only: dirtyContainer to lastEmittedDirty

e675d34

Consolidate two updateDocumentDirtyState calls

e82de84

Final clean up

fdbd24b

Copilot AI review requested due to automatic review settings May 16, 2025 22:07

github-actions bot added area: runtime Runtime related issues base: main PRs targeted against main branch labels May 16, 2025

Copilot AI reviewed May 16, 2025

View reviewed changes

revert .only

7d206af

markfields requested review from anthony-murphy, a team, steffenloesch, pragya91, jason-ha, jatgarg, kian-thompson, WillieHabi, MarioJGMsoft and vladsud May 16, 2025 22:10

anthony-murphy reviewed May 16, 2025

View reviewed changes

packages/runtime/container-runtime/src/containerRuntime.ts Show resolved Hide resolved

Fix unit tests

42cb585

anthony-murphy approved these changes May 16, 2025

View reviewed changes

markfields added 2 commits May 17, 2025 22:04

Hack in previous behavior around non-dirtyable messages.

44ea39f

The more correct fix is to compute dirty state based on isDirtyable logic every time

Revert "Hack in previous behavior around non-dirtyable messages."

18c3c56

This reverts commit 44ea39f.

Initial try at checking for dirtyable messages every time

e96048b

markfields added 2 commits May 20, 2025 15:20

Try accounting for non-dirtyable messages in isDirty calculation

49ff2fe

It seems tests still fail

Reset state to 49ff2fe

8711cb1

Right before updating isDirty to calculate

markfields marked this pull request as draft May 20, 2025 16:32

markfields added 5 commits May 20, 2025 18:01

Revert change to throw when entering SM if detached

0514a94

Clean up - new less-changy change

37fbd20

Remove .only

8b18c89

Remove redundant if checks

1dbd514

Consolidate two updateDocumentDirtyState calls

f61beb0

markfields commented May 20, 2025

View reviewed changes

packages/runtime/container-runtime/src/containerRuntime.ts Show resolved Hide resolved

markfields commented May 20, 2025

View reviewed changes

markfields added 2 commits May 20, 2025 19:03

Revert one "if" removal, and add comments

fd2eb2c

Some ID Allocation op fixes

178951c

markfields mentioned this pull request May 21, 2025

improvement(container-runtime): For resubmit, don't submit ID Allocation op until submitting another op #24545

Draft

markfields added 4 commits May 21, 2025 19:55

Check for dirtyable messages every time

eecb75e

Fix missing check for Immediate mode

ec0c14c

Fix UTs

ce87951

Revert ID Allocation "fix" in replay path

c947d2e

It shouldn't be necessary anymore since the dirty state will be correct since we're computing it robustly.

anthony-murphy added the Feature_StagingMode label May 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

improvement(container-runtime): Simplify dirty state management in ContainerRuntime #24646

improvement(container-runtime): Simplify dirty state management in ContainerRuntime #24646

markfields commented May 16, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

markfields commented May 16, 2025 •

edited

Loading

Uh oh!

anthony-murphy commented May 19, 2025

Uh oh!

markfields commented May 20, 2025

Uh oh!

Uh oh!

markfields May 20, 2025

Uh oh!

markfields May 21, 2025

Uh oh!

markfields commented May 21, 2025

Uh oh!

Uh oh!

improvement(container-runtime): Simplify dirty state management in ContainerRuntime #24646

Are you sure you want to change the base?

improvement(container-runtime): Simplify dirty state management in ContainerRuntime #24646

Conversation

markfields commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Guidance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

markfields commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anthony-murphy commented May 19, 2025

Uh oh!

markfields commented May 20, 2025

Uh oh!

Uh oh!

markfields May 20, 2025

Choose a reason for hiding this comment

Uh oh!

markfields May 21, 2025

Choose a reason for hiding this comment

Uh oh!

markfields commented May 21, 2025

Uh oh!

Uh oh!

markfields commented May 16, 2025 •

edited

Loading

markfields commented May 16, 2025 •

edited

Loading