Fix race condition when starting proofreading with a split action #8676
Conversation
This reverts commit c2d0e97.
…en a saved state should be ensured
📝 WalkthroughWalkthroughThis update refactors save request handling and testing for tracing features. It introduces a new utility for sending save requests with authentication tokens, updates action handling in sagas for improved synchronization, and enhances test fixtures and helpers for consistent ID usage. Several test files are refactored for integration-style saga testing. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code Graph Analysis (1)frontend/javascripts/test/api/api_skeleton_latest.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…cheduling the saga manually; also refactor tests a bit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/admin/rest_api.ts (1)
2403-2410: Well-implemented centralized save request function.The function properly uses
doWithTokenfor authentication and handles the JSON request correctly. The comment about ideally generating the full URL internally is a good architectural note for future refactoring.Consider the architectural improvement mentioned in the comment:
-export function sendSaveRequestWithToken( - urlWithoutToken: string, - data: RequestOptionsWithData<Array<SaveQueueEntry>>, -): Promise<void> { - // Ideally, this function should be refactored further so that it generates the - // correct urlWithoutToken itself. - return doWithToken((token) => Request.sendJSONReceiveJSON(`${urlWithoutToken}${token}`, data)); -} +export function sendSaveRequestWithToken( + tracingStoreUrl: string, + annotationId: string, + data: RequestOptionsWithData<Array<SaveQueueEntry>>, +): Promise<void> { + return doWithToken((token) => + Request.sendJSONReceiveJSON( + `${tracingStoreUrl}/tracings/annotation/${annotationId}/update?token=${token}`, + data + ) + ); +}This would eliminate the need for callers to construct the URL prefix and make the function more self-contained.
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
130-453: Consider migrating remaining manual saga tests to integration style.The file now contains a mix of integration-style tests (lines 84-128) and manual saga generator tests (lines 130-453). While this might be intentional for gradual migration, maintaining two different testing approaches in the same file could lead to confusion and maintenance overhead.
Consider either:
- Migrating all tests to the integration style for consistency
- Splitting into separate files if both approaches need to be maintained
- Adding a comment explaining why both approaches are kept
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/libs/__snapshots__/nml.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
frontend/javascripts/admin/rest_api.ts(3 hunks)frontend/javascripts/libs/utils.ts(1 hunks)frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts(2 hunks)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts(1 hunks)frontend/javascripts/test/fixtures/tasktracing_server_objects.ts(7 hunks)frontend/javascripts/test/fixtures/volumetracing_object.ts(1 hunks)frontend/javascripts/test/fixtures/volumetracing_server_objects.ts(1 hunks)frontend/javascripts/test/global_mocks.ts(1 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(4 hunks)frontend/javascripts/test/libs/nml.spec.ts(1 hunks)frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts(4 hunks)frontend/javascripts/test/sagas/saga_integration.mock.ts(0 hunks)frontend/javascripts/test/sagas/saga_integration.spec.ts(0 hunks)frontend/javascripts/test/sagas/save_saga.spec.ts(4 hunks)frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts(2 hunks)frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts(0 hunks)frontend/javascripts/test/sagas/volumetracing/bucket_eviction_with_saving.spec.ts(0 hunks)frontend/javascripts/test/sagas/volumetracing/bucket_eviction_without_saving.spec.ts(0 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(2 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(0 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts(0 hunks)frontend/javascripts/viewer/model.ts(3 hunks)frontend/javascripts/viewer/model/actions/save_actions.ts(3 hunks)frontend/javascripts/viewer/model/sagas/annotation_saga.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/save_saga.ts(5 hunks)
💤 Files with no reviewable changes (7)
- frontend/javascripts/test/sagas/volumetracing/bucket_eviction_with_saving.spec.ts
- frontend/javascripts/test/sagas/volumetracing/bucket_eviction_without_saving.spec.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts
- frontend/javascripts/test/sagas/saga_integration.spec.ts
- frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts
- frontend/javascripts/test/sagas/saga_integration.mock.ts
🧰 Additional context used
🧬 Code Graph Analysis (8)
frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts (1)
frontend/javascripts/test/fixtures/volumetracing_object.ts (1)
VOLUME_TRACING_ID(6-6)
frontend/javascripts/admin/rest_api.ts (3)
frontend/javascripts/libs/request.ts (1)
RequestOptionsWithData(33-35)frontend/javascripts/viewer/store.ts (1)
SaveQueueEntry(399-409)frontend/javascripts/admin/api/token.ts (1)
doWithToken(39-74)
frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (2)
frontend/javascripts/test/fixtures/tasktracing_server_objects.ts (1)
tracing(9-69)frontend/javascripts/types/api_types.ts (1)
ServerSkeletonTracing(904-914)
frontend/javascripts/test/fixtures/tasktracing_server_objects.ts (3)
frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (1)
annotation(167-219)frontend/javascripts/test/fixtures/volumetracing_server_objects.ts (1)
annotation(75-126)frontend/javascripts/types/api_types.ts (1)
APIAnnotation(584-586)
frontend/javascripts/viewer/model.ts (1)
frontend/javascripts/viewer/model/actions/save_actions.ts (1)
ensureTracingsWereDiffedToSaveQueueAction(130-134)
frontend/javascripts/test/helpers/apiHelpers.ts (2)
frontend/javascripts/viewer/store.ts (1)
SaveQueueEntry(399-409)frontend/javascripts/admin/rest_api.ts (1)
sendSaveRequestWithToken(2403-2410)
frontend/javascripts/test/sagas/save_saga.spec.ts (1)
frontend/javascripts/admin/rest_api.ts (1)
sendSaveRequestWithToken(2403-2410)
frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts (4)
frontend/javascripts/test/helpers/apiHelpers.ts (2)
WebknossosTestContext(47-56)setupWebknossosForTesting(193-253)frontend/javascripts/viewer/model/actions/save_actions.ts (1)
discardSaveQueuesAction(75-78)frontend/javascripts/viewer/model/sagas/root_saga.ts (1)
hasRootSagaCrashed(50-52)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
createNodeAction(189-213)
🔇 Additional comments (28)
frontend/javascripts/viewer/model/actions/save_actions.ts (2)
26-28: LGTM! New action type properly integrated.The new
EnsureTracingsWereDiffedToSaveQueueActiontype is correctly defined and added to theSaveActionunion type, maintaining type safety.Also applies to: 40-41
129-134: Well-implemented ping-pong mechanism for race condition fix.The new action creator follows the established callback pattern used by
undoActionandredoAction, providing a clean solution for the synchronization issue described in the PR objectives. The callback signature withtracingIdparameter aligns well with the diffing saga requirements.frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts (2)
177-184: LGTM! Function call updated consistently with import change.The function call has been properly updated to use the new
api.sendSaveRequestWithTokenwhile maintaining the same URL construction and request options.
14-14:✅ Verification successful
Verify the API compatibility for the refactored save request function.
The import has been changed from
sendRequestWithToken(save_saga) tosendSaveRequestWithToken(admin/rest_api). Ensure the new function maintains the same signature and behavior.
🏁 Script executed:
#!/bin/bash # Description: Verify the signature compatibility between old and new save request functions # Search for the new function definition in admin/rest_api echo "=== New function signature in admin/rest_api ===" ast-grep --pattern 'export $_ sendSaveRequestWithToken($$$) { $$$ }' # Search for any remaining usage of the old function echo "=== Check for remaining usage of sendRequestWithToken ===" rg "sendRequestWithToken" --type tsLength of output: 340
The initial ast-grep didn’t locate the new function definition. Let’s search for both functions to compare their signatures:
🏁 Script executed:
#!/bin/bash echo "=== sendSaveRequestWithToken occurrences ===" rg -n "sendSaveRequestWithToken" -A 5 -B 2 --type ts echo "=== sendRequestWithToken occurrences ===" rg -n "sendRequestWithToken" -A 5 -B 2 --type tsLength of output: 8024
API compatibility confirmed for sendSaveRequestWithToken
The new function in frontend/javascripts/admin/rest_api.ts
export function sendSaveRequestWithToken( urlWithoutToken: string, data: RequestOptionsWithData<Array<SaveQueueEntry>>, ): Promise<void> { … }matches all existing call sites (sagas, e2e tests, and unit tests), and there are no remaining references to the old sendRequestWithToken. This refactoring maintains the same signature and behavior—no further changes needed.
frontend/javascripts/test/fixtures/volumetracing_object.ts (2)
6-6: Good refactoring: Centralized tracing ID constant.Extracting the volume tracing ID into a named constant improves maintainability and consistency across the codebase.
15-15: LGTM! Property updated to use the centralized constant.The
tracingIdproperty now correctly references the exportedVOLUME_TRACING_IDconstant, maintaining consistency.frontend/javascripts/test/fixtures/volumetracing_server_objects.ts (1)
8-8: LGTM! Improved naming convention for consistency.The renaming from
"tracingId-1234"to"volumeTracingId-1234"aligns with the explicit naming conventions used in other test fixtures, making the tracing type more clear and consistent.frontend/javascripts/test/libs/nml.spec.ts (1)
558-560: LGTM! Test expectation updated to match standardized ID format.The test correctly expects the new filename format with the "taskId-" prefix, which aligns with the broader standardization of ID formats across test fixtures.
frontend/javascripts/libs/utils.ts (1)
1307-1313: LGTM! Well-implemented set equality utility function.The implementation is efficient with early size checking and correctly compares set equality. The generic typing makes it reusable across different data types.
frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts (2)
9-9: LGTM! Good refactoring to use centralized test constants.Importing the
VOLUME_TRACING_IDconstant improves test maintainability and consistency.
116-116: LGTM! Consistent replacement of hardcoded strings with constants.Replacing hardcoded
"tracingId"strings with theVOLUME_TRACING_IDconstant eliminates potential typos and improves test maintainability. All replacements are consistent and appropriate.Also applies to: 163-163, 233-233
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
314-320: LGTM! Improves test error visibility without affecting production.The conditional error logging for test environments addresses a real issue where console.error output after yield might not be visible due to test assertions throwing. The change is well-commented and isolated to test environments only.
frontend/javascripts/test/global_mocks.ts (1)
116-143: LGTM! Well-structured UI component mocks.The antd mock provides comprehensive coverage of commonly used UI components with appropriate stub implementations. The message methods correctly return hide functions matching the real API, and the simple mock for
libs/render_independentlyis suitable for test isolation.frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts (2)
8-8: Good practice: Using semantic ID prefixes for test consistency.The prefixed ID format improves clarity about the entity type and aligns with similar updates across other test fixtures.
12-12: LGTM! Using constant instead of hardcoded string.Referencing the
TRACING_IDconstant improves maintainability and reduces duplication.frontend/javascripts/test/sagas/save_saga.spec.ts (2)
21-21: LGTM! Updated import for centralized save request function.Correctly imports the new
sendSaveRequestWithTokenfunction from the admin REST API module.
142-142: Consistent update across all test cases.All test references have been properly updated to use the centralized
sendSaveRequestWithTokenfunction while maintaining the same call signature and behavior.Also applies to: 165-165, 213-213
frontend/javascripts/admin/rest_api.ts (1)
3-3: LGTM! Proper type imports for the new function.The
RequestOptionsWithDataandSaveQueueEntrytypes are correctly imported to support the newsendSaveRequestWithTokenfunction.Also applies to: 93-93
frontend/javascripts/test/helpers/apiHelpers.ts (3)
32-32: LGTM! Appropriate imports for test mocking.The imports are correctly structured to support mocking the save request functionality with proper type safety.
Also applies to: 40-40
72-85: Well-structured mock implementation for save request tracking.The mock effectively captures all save request payloads in
receivedDataPerRequestarray, enabling tests to verify the data sent through the saga. The approach of attaching the array to the mock function is a clean pattern for test data access.
55-55: Clean integration of mock data into test context.The interface extension and test context setup provide type-safe access to the captured save request data, making it straightforward for tests to verify saga behavior.
Also applies to: 212-212
frontend/javascripts/test/fixtures/tasktracing_server_objects.ts (1)
8-8: Good improvement to test fixture clarity with ID prefixes.The addition of descriptive prefixes to all IDs (e.g., "skeletonTracingId-", "datasetId-", etc.) enhances test readability and debugging. The comment clarifying the empty name test case is also helpful.
Also applies to: 45-45, 71-71, 74-74, 80-80, 88-88, 101-101, 159-159, 168-168, 179-179
frontend/javascripts/viewer/model.ts (2)
13-16: Appropriate imports for the synchronization mechanism.The imports provide the necessary actions and utilities for implementing the ping-pong synchronization between the model and sagas.
Also applies to: 25-25
346-388: Excellent implementation of the ping-pong synchronization mechanism.The new
ensureSavedStateimplementation effectively addresses the race condition by:
- Ensuring all diffing sagas have processed updates before checking save state
- Using a callback mechanism to track responses from all active tracings
- Properly handling both skeleton and volume tracings with
compact()to filter nulls- Using
areSetsEqualfor robust set comparisonThe solution is clean, well-documented, and should reliably prevent the premature return issue described in the PR objectives.
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
84-128: Great refactoring to integration-style testing!The migration from manual saga generator testing to integration tests using real saga middleware provides better confidence in the actual behavior of the system. The tests now properly verify the real save requests sent to the backend.
frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts (1)
140-179: Well-implemented integration tests with proper saga lifecycle management.The integration tests correctly:
- Set up the Webknossos environment with skeleton tracing
- Dispatch necessary actions to initialize the sagas
- Use
Model.ensureSavedState()to wait for save completion- Verify the actual save requests sent to the backend
- Clean up resources and verify saga health after each test
frontend/javascripts/viewer/model/sagas/save_saga.ts (2)
375-411: Excellent implementation of the ping-pong synchronization mechanism!The refactored
setupSavingForTracingTypesaga effectively addresses the race condition by:
- Using an expanding buffer to queue tracing actions, preventing action loss
- Creating a separate channel for
ENSURE_TRACINGS_WERE_DIFFED_TO_SAVE_QUEUEactions- Prioritizing user actions over synchronization requests to ensure all pending changes are processed
This implementation correctly ensures that
ensureSavedStatewaits for all diffing sagas to complete before returning.
413-429: Smart prioritization logic prevents race conditions.The conditional logic that prioritizes consuming buffered user actions before responding to synchronization requests is well-designed. This ensures that
ensureSavedStateonly gets a response after all pending tracing actions have been processed and diffed.The use of
racewhen the buffer is empty allows the saga to respond to either new user actions or synchronization requests, maintaining responsiveness.
| // Setup Webknossos | ||
| // this will execute model.fetch(...) and initialize the store with the tracing, etc. | ||
| Store.dispatch(restartSagaAction()); | ||
| Store.dispatch(discardSaveQueuesAction()); | ||
| Store.dispatch(setActiveUserAction(dummyUser)); | ||
| await setupWebknossosForTesting(context, "volume"); | ||
|
|
||
| // Dispatch the wkReadyAction, so the sagas are started | ||
| Store.dispatch(wkReadyAction()); |
There was a problem hiding this comment.
At some point it probably makes sense to extract this and the teardown code into a test helper function, but this is not a blocker for this PR
There was a problem hiding this comment.
good point! I found that I can integrate the code simply into setupWebknossosForTesting.
Co-authored-by: Daniel <daniel.werner@scalableminds.com>
…s into fix-await-saving
Don't wait for differ response if there are no tracings, for example in view mode. ### URL of deployed dev instance (used for testing): - https://fixmappingactivationviewmode.webknossos.xyz ### Steps to test: - Open a dataset in view mode and activate a mapping. Previously, the mapping activation was stuck and the mapping never got activated. - Follow the testing instructions in #8676 to check that the race condition is still fixed ------ (Please delete unneeded items, merge only when none are left open) - [x] Updated [changelog](../blob/master/CHANGELOG.unreleased.md#unreleased) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) --------- Co-authored-by: Philipp Otto <philipp.4096@gmail.com>
Agglomerate Mappings can now also be read from the new zarr3-based format, and from remote object storage. - AgglomerateFileKey, which identifies an agglomerate file, now holds a LayerAttachment (which can specify a remote URI) so we can use VaultPaths for accessing remote agglomerate files - interface of AgglomerateService methods changed (take the new AgglomerateFileKey) AgglomerateService is no longer injected, instead it is explicitly created in BinaryDataServiceHolder so we can pass it the sharedChunkContentsCache - AgglomerateService now delegates to either Hdf5AgglomerateService (basically the old code) or ZarrAgglomerateService (a lot of duplication from the other one, but unifying it at this time would sacrifice performance for hdf5) - DatasetArray has new public method ReadAsMultiArray, which does not do any axisorder but instead yields a MultiArray in the order of the DatasetArray - removed unused route agglomerateIdsForAllSegmentIds ### URL of deployed dev instance (used for testing): - https://zarragglomerates.webknossos.xyz ### Steps to test: - With test dataset from https://www.notion.so/scalableminds/Test-Datasets-c0563be9c4a4499dae4e16d9b2497cfb?source=copy_link#209b51644c6380ac85e0f6b0c7e339cf select agglomerate mapping, should be displayed correctly - Import agglomerate skeleton, should look plausible - Do some proofreading (splits + merges) should work - Also test that the old format still works (e.g. use older test-agglomerate-file dataset with hdf5 agglomerate files) ### TODOs: <details> <summary>Backend</summary> - [x] open zarr agglomerate as zarr array and read contents - [x] read MultiArray without caring about AxisOrder - [x] test with 2D - [x] Read agglomerate arrays with correct types - [x] Re-Implement public functions of agglomerate service - [x] applyAgglomerate - [x] generate agglomerate skeleton - [x] largest agglomerate id - [x] generate agglomerate graph - [x] segmentIdsForAgglomerateId - [x] agglomerateIdsForSegmentIds - [x] positionForSegmentId - [x] What’s up with zarr streaming in the tests? reproduce with test-dataset, ids are wrong, also in normal data loading - [x] Create indirection for selecting the zarr agglomerates OR hdf5 agglomerates - [x] reduce code duplication btw hdf5 and zarr - [x] Error handling (index lookups always in tryo. abstraction?) - [x] Read remote (Build VaultPath for URI) - [x] Discover files? - [x] Adapt requests to specify which agglomerate file should be read from (type? full path?) - [x] Caching / Speedup (added some caching but did not test on large-scale DS. will be follow-up) - [x] Clear caches on layer/DS reload - [x] Make sure the agglomerate zarr directories don’t blow up dataset exploring - [x] Code Cleanup </details> <details> <summary>Frontend</summary> - [x] Starting proofreading with a split action doesn’t properly flush updateMappingName before calling minCut route → fixed by #8676 </details> ### Issues: - contributes to #8618 - contributes to #8567 ------ - [x] Updated [changelog](../blob/master/CHANGELOG.unreleased.md#unreleased) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [x] Needs datastore update after deployment
When splitting an agglomerate in the proofreading mode while being in a saved state and without any editable mapping yet being created, wk used to crash.
What happened? The mapping name was updated in the store. This would trigger the sagas that push update actions into the save queue. However, this did not happen synchronously. Instead, the proofreading saga would immediately call "ensureSavedState". As the save queue was not filled yet, this function call would immediately return.
As a solution to this problem, this PR implements a ping-pong mechanism in which "ensureSavedState" dispatches an action to which all diffing sagas have to respond. By responding to the action, we know that the diffing sagas have already run.
An alternative approach would have been to update some sort of version counter every time the annotation in the store changes. Then, the diffing sagas could store the newest version they have seen somewhere. The downside of this approach is that we would need to store these versions (and the "seen versions") per tracing. also, lots of store updates for these counters would happen. All of this can be avoided with the other approach described above.
Additionally, this PR changes 4 saga tests from a generator-style to an integrated style where the saga middleware is used.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)