fix(editor): serialize structural block ops through DatabaseWriteActor#97
Merged
Conversation
Fast typing followed by Enter caused new blocks to disappear or revert because splitBlock/mergeBlock/handleBackspace called blockRepository directly, bypassing the actor queue. A pending updateBlockContentOnly enqueued in the actor would execute after the structural op, overwriting the split/merge result with stale content. Fix: add typed splitBlock/mergeBlocks/deleteBlockStructural methods to DatabaseWriteActor and route all structural call sites in BlockStateManager through the actor via private writeSplitBlock/writeMergeBlocks/writeDeleteBlockStructural helpers (writeActor?.xxx() ?: blockRepository.xxx() fallback for tests). Also adds hasPendingWrites (AtomicInt counter) to DatabaseWriteActor and exposes hasActorPendingWrites on BlockStateManager so conflict detection in StelekitViewModel catches external file changes that arrive while a split/merge is in-flight. Adds 4 race-condition tests that verify ordering under concurrent content-write + structural-op scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
JVM Load Benchmark (Desktop)Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Flamegraphs (this PR)**Allocation** — object allocation pressure (JDBC/SQLite churn)Alloc flamegraph not available CPU — method-level hotspots by on-CPU time CPU flamegraph not available Top SQL queries by total time (this PR)| table:operation | calls | p50 | p99 | max | total | |-----------------|-------|-----|-----|-----|-------| | `pages:select` | 2 | 1ms | 1ms | 1ms | 1ms |Top allocation hotspots (this PR)`71.6%` byte[]_[k] `5.6%` java.lang.String_[k] `2.5%` java.lang.StringBuilder_[k] `1.9%` jdk.internal.org.objectweb.asm.SymbolTable$Entry_[k] `1.2%` jdk.internal.ref.CleanerImpl$PhantomCleanableRef_[k]Top CPU hotspots (this PR)`99.4%` /usr/lib/x86_64-linux-gnu/libc.so.6 `0.1%` /tmp/sqlite-3.51.3.0-9a34a6c3-1d9e-4eb2-9741-05029cb41a6d-libsqlitejdbc.so `0%` kotlinx/coroutines/scheduling/CoroutineScheduler$Worker.trySteal_[0] `0%` java/util/zip/ZipFile$Source.getEntryPos_[i] `0%` kotlinx/coroutines/internal/DispatchedContinuation._[0] |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the editor where structural operations (split/merge/delete) could bypass the DatabaseWriteActor queue and interleave incorrectly with pending content writes, causing blocks to disappear or content to revert under fast typing + Enter/Backspace.
Changes:
- Route structural block operations (
splitBlock,mergeBlocks, structural deletes) throughDatabaseWriteActorto serialize them with content writes. - Add an actor “pending writes” flag and use it in external file-change conflict protection to avoid overwriting queued structural edits.
- Add deterministic race-condition tests using a delayed
updateBlockContentOnlyrepository wrapper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/db/DatabaseWriteActor.kt | Adds typed structural methods, injectable scope for tests, and a pending-writes indicator. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/state/BlockStateManager.kt | Routes structural operations through actor-backed helper methods and exposes hasActorPendingWrites. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/StelekitViewModel.kt | Extends external-file-change conflict guard to include actor-queue pending structural writes. |
| kmp/src/commonTest/kotlin/dev/stapler/stelekit/ui/state/BlockStateManagerTest.kt | Adds 4 race-condition regression tests and a delayed content-write repository test double. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Android Load BenchmarkInstrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph. Comparing Graph Load
Interactive Write Latency (during Phase 3)
SAF I/O Overhead (ContentProvider vs direct File read)Measures Binder IPC cost added by ContentResolver per readFile() call.
|
…ased hasPendingWrites Two issues flagged in PR review: 1. `_activeOps` used `@Volatile Int` with `++`/`--` — not atomic; concurrent `execute()` callers could corrupt the count or make it go negative. 2. Counter was incremented before `send()` but decremented only after `await()`. If `send()` threw (closed channel, cancellation), the counter leaked indefinitely. Fix: drop the explicit counter entirely. `hasPendingWrites` now derives state from the channels themselves (`!highPriority.isEmpty || !lowPriority.isEmpty`) plus an actor-owned `@Volatile Boolean` flag set inside the `processRequest` try/finally. Channel.isEmpty is concurrency-safe; the flag is single-writer (actor coroutine), so @volatile gives correct multi-reader visibility without atomics or new deps. Also adds the ClassLevelDirectRepositoryWriteOptIn detekt rule (post-mortem enforcement) and moves class-level @OptIn to function-level across BlockStateManager, StelekitViewModel, AnnotationEditorViewModel, BacklinkRenamer, and GlobalUnlinkedReferencesViewModel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… scope ownership, watcher correctness Addresses all 31 findings from the post-architecture code review. Key changes: - GraphLoaderPort / GraphWriterPort: replace mutable var properties with explicit setter functions; add renamePage/savePage/deletePage to port so callers never need unsafe casts to the concrete type - BlockRepository split into BlockReadRepository, BlockWriteRepository, BlockSearchRepository, BlockStructureRepository (ISP) - ImportViewModel / ScreenRouter: class now owns its CoroutineScope; removed rememberCoroutineScope() leaking out of composition (C18 fix) - GraphFileWatcher: fix SharedFlow suppression (Channel.RENDEZVOUS replaces broken yield() pattern), add close() to cancel owned scope, add pollIntervalMs parameter, log dropped events (C13/C14/C15/A3) - MarkdownPageParser: extract to own file, replace Pair/Triple returns with typed PageBuildResult/PageMetadata, deduplicate mergedProperties, remove dead title alias - FlashcardScheduler: extract SM-2 logic from @composable into pure stateless object - GraphEvents.kt: move ExternalFileChange and WriteError out of GraphLoader - AllPagesViewModel: depend on BlockSearchRepository, not full BlockRepository - BlockWriteRepository.clear(): return Either<DomainError, Unit> - Remove unused count field from DuplicateGroup - StelekitViewModelDependencies: remove default scope value, require explicit supply - GraphLoaderWatcherTest: replace vacuous assertTrue(count >= 0) with real assertion - GraphFileWatcherTest: new suite covering suppression, close(), git-merge paths; fix virtual-time/real-time timeout mismatch and registry priming Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Audit all BlockRepository usages and apply ISP — classes that only read now depend on BlockReadRepository; write-only on BlockWriteRepository. Removes the dead blockRepository parameter from JournalsView (unused). Files narrowed to BlockReadRepository: VectorSearch, ExportService, Editor Files narrowed to BlockWriteRepository: TextOperations, ImageImportService Unused imports removed: DatalogQuery, BlockTreeOperations Dead parameter removed: JournalsView + 9 call sites updated Files kept as BlockRepository: JournalService, BacklinkRenamer, ReferencesPanel, BlockStateManager, EditorViewModel, StelekitViewModel, DatabaseWriteActor, PersistenceManager, OptimizedTextOperations, GlobalUnlinkedReferencesViewModel (all use multiple roles legitimately) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
resolvedDisplay() checks $DISPLAY first, then scans /tmp/.X*-lock to find a running XWayland instance. Fixes all 78 UI test failures when running from a terminal on Wayland where $DISPLAY is not exported to the shell. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces single resolvedDisplay() with four helpers: - linuxUid(): reads effective UID from /proc/self/status - resolvedXdgRuntimeDir(): env var → /proc UID probe - resolvedDisplay(): env var → XWayland /tmp/.X*-lock probe - resolvedWaylandDisplay(): env var → wayland-0 socket probe configureDisplayEnv() applies all three to a Test task so the JVM receives whichever display variables are discoverable: DISPLAY for AWT, WAYLAND_DISPLAY + XDG_RUNTIME_DIR for Skiko's Wayland renderer. Works on X11, Wayland+XWayland, and CI with a real display. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eIntStateOf, nesting depth, unused param Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…leak in DatabaseWriteActor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the flawed `committedAnnotations.isEmpty()` guard in `initialize()` with an explicit `hasBeenMutated` AtomicBoolean flag. The old guard could not distinguish "no annotations ever added" from "user deleted all annotations", causing the background init coroutine to restore a just-deleted annotation from the repository. Now `commitAnnotation` and `deleteAnnotation` both set `hasBeenMutated = true` before touching state, and the init coroutine skips applying repository results once any mutation has occurred. Also adds an `initialized` flag (via `compareAndSet`) to prevent a second `initialize()` call from re-issuing the one-shot repository load. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fast typing followed immediately by Enter caused new blocks to disappear, content to revert to a stale version, or blocks to merge incorrectly (reproducible >50% of the time).
Root cause
BlockStateManagerhas two uncoordinated async write paths:applyContentChange→writeActor.updateBlockContentOnly()— queued throughDatabaseWriteActor(serialized)splitBlock,addNewBlock,mergeBlock,handleBackspace— calledblockRepository.splitBlock/mergeBlocks/deleteBlockdirectly, bypassing the actor queueRace sequence:
Fix
DatabaseWriteActor— adds three typed methods that route throughexecute {}:splitBlock(blockUuid, cursorPosition, newBlockUuid)mergeBlocks(keepUuid, dropUuid, separator)deleteBlockStructural(blockUuid)Also adds
hasPendingWrites(@Volatilecounter covering the full enqueue→DB-commit window).BlockStateManager— adds privatewriteSplitBlock/writeMergeBlocks/writeDeleteBlockStructuralhelpers using the establishedwriteActor?.xxx() ?: blockRepository.xxx()fallback pattern. All 6 structural call sites acrossaddNewBlock,splitBlock,mergeBlock, andhandleBackspacenow route through these helpers.StelekitViewModel— extends the external-file-change conflict guard from 3-tier to 4-tier: addshasActorPendingWritesas a fourth condition so a file watcher event arriving while a split/merge is in the actor queue triggers the conflict dialog instead of silently overwriting.Tests
Added 4 race-condition tests using
DelayedContentBlockRepository(delaysupdateBlockContentOnlyto create a deterministic race window):splitBlock_after_pending_content_write_uses_latest_contentaddNewBlock_after_pending_content_write_preserves_typed_contentmergeBlock_after_pending_content_write_uses_latest_contenthandleBackspace_after_pending_content_write_merges_latest_contentTest results
BlockStateManagerTest: 60/60 PASS (56 pre-existing + 4 new)DatabaseWriteActorTest: 18/18 PASSdetekt: CLEAN🤖 Generated with Claude Code