Skip to content

Conversation

@dariacodes
Copy link
Contributor

@dariacodes dariacodes commented Dec 15, 2025

Summary

Description

Implement write locks for users editing workflows. The lock is released after 20 seconds of inactivity.

  • Fixed the sharing e2e by opening workflows page after each test (this releases the write lock)

Explanation of some solutions/decisions:

  1. The workflowUpdated event is handled directly in the collaboration store instead of going through the usePushConnection/handlers pattern because it needs collaboration-specific state (like write locks, spectator mode and canvas refresh callbacks). It also has a different lifecycle - it’s tied to the CollaborationPane component rather than being global.

  2. The collaboration store doesn’t talk to the canvas directly. Instead, it exposes a callback that the canvas hooks into. This avoids circular dependencies and keeps a clean split between business logic (the store) and view logic (the component). The store decides when a refresh should happen, and the NodeView handles how it happens, like restoring UI state or reopening the NDV.

  3. Fix stuck workflow write locks with TTL + heartbeat + expiration detection
    When a writer’s tab crashes/goes offline, the workflow write lock could previously remain held indefinitely, blocking collaborators until manual backend intervention. So these were introduced:

    • Backend write-lock TTL: 120s (2 minutes i.e. allows ~3 missed heartbeats) - all write locks in cache expire automatically unless renewed.
    • Writer heartbeat: every 30s the lock holder sends a writeAccessHeartbeat message to renew the TTL.
    • Spectator polling: every 20s read-only clients poll lock state and clear stale currentWriterId when the backend lock has expired.
  4. We send telemetry for audit in log-streaming.event-relay where we could put workflowUpdated, but as it is only needed for collaboration to work correctly, it is not needed here.

Related Linear tickets, Github issues, and Community forum posts

Closes ADO-4284

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 15, 2025
@bundlemon
Copy link

bundlemon bot commented Dec 15, 2025

BundleMon

Unchanged files (2)
Status Path Size Limits
WASM Dependencies
tree-sitter-bash.wasm
181.26KB -
WASM Dependencies
tree-sitter.wasm
74.47KB -

No change in files bundle size

Groups updated (1)
Status Path Size Limits
**/*.js
6.05MB (+3.33KB +0.05%) -
Unchanged groups (1)
Status Path Size Limits
**/*.css
240.38KB -

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@blacksmith-sh

This comment has been minimized.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 21 files

@currents-bot
Copy link

currents-bot bot commented Dec 15, 2025

E2E Tests: n8n tests passed after 9m 44.2s

🟢 618 · 🔴 0 · ⚪️ 26 · 🟣 4

View Run Details

Run Details

  • Project: n8n

  • Groups: 2

  • Framework: Playwright

  • Run Status: Passed

  • Commit: 163f177

  • Spec files: 140

  • Overall tests: 644

  • Duration: 9m 44.2s

  • Parallelization: 16

Groups

GroupId Results Spec Files Progress
multi-main:e2e:isolated 🟢 57 · 🔴 0 · ⚪️ 0 9 / 9
multi-main:e2e 🟢 561 · 🔴 0 · ⚪️ 26 · 🟣 4 131 / 131


This message was posted automatically by currents.dev | Integration Settings

@dariacodes dariacodes marked this pull request as draft December 15, 2025 15:28
@dariacodes dariacodes force-pushed the ado-4284-autosave-collaboration branch 4 times, most recently from dbb9926 to 9e88f87 Compare December 16, 2025 15:17
@blacksmith-sh

This comment has been minimized.

@dariacodes dariacodes force-pushed the ado-4284-autosave-collaboration branch 3 times, most recently from b7c0b4d to 535a8bf Compare December 17, 2025 11:30
@dariacodes dariacodes force-pushed the ado-4284-autosave-collaboration branch from 3bb7271 to a943d05 Compare December 17, 2025 11:39
@dariacodes dariacodes force-pushed the ado-4284-autosave-collaboration branch from 5bdfe95 to f5edf4c Compare December 18, 2025 13:05
@blacksmith-sh

This comment has been minimized.

@dariacodes dariacodes marked this pull request as ready for review January 7, 2026 14:44
@blacksmith-sh

This comment has been minimized.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/cli/src/collaboration/collaboration.service.ts">

<violation number="1" location="packages/cli/src/collaboration/collaboration.service.ts:175">
P1: Heartbeat renewal should require *write* access, not just read access, otherwise a user who no longer has write permissions could keep a write lock alive and block editors.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@blacksmith-sh

This comment has been minimized.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue">

<violation number="1" location="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue:139">
P2: Autosave handling may be skipped when dirty state is false. If an autosave is in progress but `stateIsDirty` has been cleared (e.g., save started but not completed), the publish modal will open without waiting for the pending save. Consider calling `saveBeforePublish()` unconditionally or at least checking autosave state before opening the modal.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@blacksmith-sh

This comment has been minimized.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue">

<violation number="1">
P2: If `saveCurrentWorkflow` throws an exception, `autoSaveForPublish.value = false` will never execute, leaving the publish button stuck in a loading state. Consider wrapping this in a try-finally block to ensure the loading state is always reset.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts">

<violation number="1" location="packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts:153">
P2: Incomplete concurrent save prevention: when a manual save is already in progress, another manual save will proceed without waiting because `pendingAutoSave` is only set for autosaves. The code should either track all in-progress saves or add an else case to handle when `pendingAutoSave` is null but `workflowSaving` is still active.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.


// Prevent concurrent saves - if a save is already in progress, skip this one
// for autosaves (they will be rescheduled), or wait for non-autosaves
if (uiStore.isActionActive.workflowSaving) {
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Incomplete concurrent save prevention: when a manual save is already in progress, another manual save will proceed without waiting because pendingAutoSave is only set for autosaves. The code should either track all in-progress saves or add an else case to handle when pendingAutoSave is null but workflowSaving is still active.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts, line 153:

<comment>Incomplete concurrent save prevention: when a manual save is already in progress, another manual save will proceed without waiting because `pendingAutoSave` is only set for autosaves. The code should either track all in-progress saves or add an else case to handle when `pendingAutoSave` is null but `workflowSaving` is still active.</comment>

<file context>
@@ -148,6 +148,19 @@ export function useWorkflowSaving({
 
+		// Prevent concurrent saves - if a save is already in progress, skip this one
+		// for autosaves (they will be rescheduled), or wait for non-autosaves
+		if (uiStore.isActionActive.workflowSaving) {
+			if (autosaved) {
+				// Autosave will be rescheduled by the finally block of the in-progress save
</file context>
Fix with Cubic

@scdekov
Copy link
Member

scdekov commented Jan 8, 2026

@claude review this PR

@claude
Copy link
Contributor

claude bot commented Jan 8, 2026

Claude finished @scdekov's task —— View job


Code Review: Autosave Collaboration Feature

Overall Assessment

This is a well-architected collaboration feature that implements write locks for shared workflow editing. The implementation follows good practices with proper separation between frontend/backend concerns and includes robust error handling. However, there are several important issues that need attention.

Review Progress

  • Analyze collaboration architecture and key components
  • Review backend collaboration service implementation
  • Review frontend collaboration store and UI integration
  • Check activity detection and workflow saving logic
  • Review API changes and type definitions
  • Assess test coverage and quality
  • Provide comprehensive feedback and recommendations

🚨 Critical Issues

1. Security Issue: Heartbeat Authentication - packages/cli/src/collaboration/collaboration.service.ts:175

The heartbeat renewal currently only checks hasReadAccess instead of hasWriteAccess. This allows users who have lost write permissions to keep write locks alive indefinitely, blocking legitimate editors.

// ❌ Current - allows read-only users to maintain locks
if (!(await this.accessService.hasReadAccess(userId, workflowId))) {
    return;
}

// ✅ Should be - requires write access to renew locks
if (!(await this.accessService.hasWriteAccess(userId, workflowId))) {
    return;
}

2. Race Condition: Autosave State Management - packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue:139

The publish modal can open while autosave is in progress if stateIsDirty has been cleared but autosave hasn't completed, potentially causing data consistency issues.


🔧 Architecture Strengths

Clean separation of concerns: Collaboration store manages business logic while UI components handle presentation
Robust TTL system: 120s write lock TTL with 30s heartbeats prevents stuck locks
Event-driven architecture: Uses push messages for real-time collaboration updates
Proper cleanup: Comprehensive timer cleanup and event listener removal
Activity detection: Debounced user activity tracking with smart inactivity detection


⚠️ Areas for Improvement

Backend Issues

  1. Error Handling: Some operations lack proper error recovery (e.g., cache failures)
  2. Logging: Console logs should use proper logging service instead of console.log
  3. Write Lock Validation: The validateWriteLock method should be used more consistently across API endpoints

Frontend Issues

  1. Memory Leaks: Multiple timer references could accumulate if cleanup fails
  2. State Synchronization: Lock state polling could create race conditions with real-time updates
  3. Error Recovery: Limited fallback mechanisms when websocket connection fails

Code Quality

  1. Test Coverage: Only 70.01% coverage with 290 missing lines, particularly in the main collaboration store
  2. Type Safety: Some any types could be more specific
  3. Documentation: Complex state transitions could benefit from more inline documentation

🧪 Testing Concerns

The automated tests show good coverage for basic scenarios but are missing:

  • Concurrent user scenarios: Multiple users requesting/releasing locks simultaneously
  • Network failure recovery: How the system behaves when connections are lost
  • Edge cases: Lock expiration during active editing, browser crashes, etc.

🔧 Specific Code Recommendations

1. Fix Heartbeat Security Issue

File: packages/cli/src/collaboration/collaboration.service.ts:175

// Change from hasReadAccess to hasWriteAccess
if (!(await this.accessService.hasWriteAccess(userId, workflowId))) {
    return;
}

2. Improve Concurrent Save Prevention

File: packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts:153

// Add manual save tracking
if (uiStore.isActionActive.workflowSaving) {
    if (autosaved) {
        return true; // Autosaves are rescheduled
    } else {
        // Wait for any pending save to complete
        await waitForActiveSaveToComplete();
    }
}

3. Enhance Activity Detection

File: packages/frontend/editor-ui/src/app/composables/useActivityDetection.ts:13

// Consider expanding activity events to include:
// - scroll events for canvas navigation
// - input events for form fields
// - focus/blur for better idle detection
const events = ['mousedown', 'keydown', 'touchstart', 'wheel', 'input'];

📝 Documentation & Maintenance

  • API Documentation: New endpoints need OpenAPI documentation
  • Deployment Notes: Cache configuration requirements should be documented
  • Monitoring: Add metrics for lock contention and collaboration session health

🎯 Priority Actions

  1. HIGH: Fix heartbeat authentication security issue
  2. HIGH: Address autosave race condition in publish flow
  3. MEDIUM: Improve test coverage, especially for edge cases
  4. MEDIUM: Replace console.log with proper logging
  5. LOW: Add comprehensive error recovery mechanisms

Summary

This is a solid implementation of a complex feature. The architecture is well-thought-out and the code follows good patterns. The main concerns are around edge cases, error handling, and the critical security issue with heartbeat authentication. Once these issues are addressed, this will be a robust collaboration system.

Recommendation: Address the critical security issue before merging, and consider the race condition fix as it could cause data consistency problems in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants