From 3f293125f55408fc1feb1e5890a654eb34553b44 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 13:35:17 +0000 Subject: [PATCH 1/2] ADFA-5231: publish document version and content together Change events are dispatched from a background coroutine per edit, so two edits in one frame raced: a non-atomic ++fileVersion, then an unsynchronised version-then-content write. A version that moved backwards made the Kotlin index mint a second KtFile for text that never changed. --- .../itsaky/androidide/editor/ui/IDEEditor.kt | 18 ++++-- .../itsaky/androidide/projects/FileManager.kt | 63 ++++++++----------- .../projects/models/ActiveDocument.kt | 53 +++++++++++++--- .../projects/ActiveDocumentVersionTest.kt | 47 ++++++++++++++ 4 files changed, 128 insertions(+), 53 deletions(-) create mode 100644 subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt index 73a0c87cdb..918d69af7e 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt @@ -104,12 +104,15 @@ import kotlinx.coroutines.cancel import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import org.slf4j.LoggerFactory import java.io.File +import java.util.concurrent.atomic.AtomicInteger import kotlin.coroutines.resume fun interface OnEditorLongPressListener { @@ -141,7 +144,8 @@ open class IDEEditor private var actionsMenu: EditorActionsMenu? = null private var _signatureHelpWindow: SignatureHelpWindow? = null private var _diagnosticWindow: DiagnosticWindow? = null - private var fileVersion = 0 + private val fileVersion = AtomicInteger(0) + private val documentChangeMutex = Mutex() internal var isModified = false // Length and content hash of the content the last time the file was loaded or saved. @@ -570,7 +574,7 @@ open class IDEEditor languageClient = null _file = null - fileVersion = 0 + fileVersion.set(0) markUnmodified() editorFeatures.editor = null @@ -960,7 +964,9 @@ open class IDEEditor file ?: return@subscribeEvent editorScope.launch { - dispatchDocumentChangeEvent(event) + // Serialised so the version a change is stamped with is never older than the text + // snapshot taken with it: two edits in one frame land here as two coroutines. + documentChangeMutex.withLock { dispatchDocumentChangeEvent(event) } checkForSignatureHelp(event) handleCustomTextReplacement(event) } @@ -1242,9 +1248,9 @@ open class IDEEditor val file = this.file ?: return - this.fileVersion = 0 + this.fileVersion.set(0) - val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion) + val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion.get()) eventDispatcher.dispatch(openEvent) } @@ -1278,7 +1284,7 @@ open class IDEEditor file, changedText, text.toString(), - ++fileVersion, + fileVersion.incrementAndGet(), type, changeDelta, changeRange, diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt index 59d6242568..c19d05f1d9 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt @@ -43,28 +43,19 @@ import java.util.concurrent.ConcurrentHashMap * @author Akash Yadav */ object FileManager { - private val log = LoggerFactory.getLogger(FileManager::class.java) private val _activeDocuments = ConcurrentHashMap() val activeDocuments: Collection get() = _activeDocuments.values.toSet() - fun isActive(uri: URI): Boolean { - return isActive(Paths.get(uri)) - } + fun isActive(uri: URI): Boolean = isActive(Paths.get(uri)) - fun isActive(file: Path): Boolean { - return this._activeDocuments.containsKey(file.normalize()) - } + fun isActive(file: Path): Boolean = this._activeDocuments.containsKey(file.normalize()) - fun getActiveDocument(file: Path): ActiveDocument? { - return this._activeDocuments[file.normalize()] - } + fun getActiveDocument(file: Path): ActiveDocument? = this._activeDocuments[file.normalize()] - fun getActiveDocumentCount(): Int { - return this._activeDocuments.size - } + fun getActiveDocumentCount(): Int = this._activeDocuments.size fun getDocumentContents(file: Path): String { val document = getActiveDocument(file) @@ -115,14 +106,19 @@ object FileManager { _activeDocuments[event.changedFile.normalize()] = createDocument(event) log.warn( "Document change event received before open event for file {}", - event.changedFile + event.changedFile, ) return } - document.version = event.version - document.modified = Instant.now() - document.content = event.newText!! + if (!document.update(event.version, event.newText!!)) { + log.debug( + "Ignoring out-of-order change for {}: event version {} is older than {}", + event.changedFile, + event.version, + document.version, + ) + } event.newText = null } @@ -142,26 +138,24 @@ object FileManager { _activeDocuments.remove(event.file.toPath().normalize()) } - private fun createDocument(event: DocumentOpenEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentOpenEvent): ActiveDocument = + ActiveDocument( file = event.openedFile, version = event.version, modified = Instant.now(), - content = event.text + content = event.text, ) - } - private fun createDocument(event: DocumentChangeEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentChangeEvent): ActiveDocument = + ActiveDocument( file = event.changedFile, version = event.version, modified = Instant.now(), - content = event.changedText + content = event.changedText, ) - } - private fun createFileReader(file: Path): BufferedReader { - return try { + private fun createFileReader(file: Path): BufferedReader = + try { Files.newBufferedReader(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -169,10 +163,9 @@ object FileManager { } catch (cancelled: CancellationException) { "".reader().buffered() } - } - private fun createFileInputStream(file: Path): InputStream { - return try { + private fun createFileInputStream(file: Path): InputStream = + try { Files.newInputStream(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -180,14 +173,11 @@ object FileManager { } catch (cancelled: CancellationException) { "".byteInputStream() } - } - private fun getLastModifiedFromDisk(file: Path): Instant { - return Files.getLastModifiedTime(file).toInstant() - } + private fun getLastModifiedFromDisk(file: Path): Instant = Files.getLastModifiedTime(file).toInstant() - private fun getFileContents(file: Path): String { - return try { + private fun getFileContents(file: Path): String = + try { ProgressManager.abortIfCancelled() FileUtils.readFileToString(file.toFile(), Charset.defaultCharset()) } catch (noFile: java.nio.file.NoSuchFileException) { @@ -196,5 +186,4 @@ object FileManager { } catch (cancelled: CancellationException) { "" } - } } diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt index 42b0b7e6cc..89bdeab453 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt @@ -29,19 +29,52 @@ import java.time.Instant */ open class ActiveDocument( val file: Path, - var version: Int, - var modified: Instant, - content: String = "" + version: Int, + modified: Instant, + content: String = "", ) { + private data class Snapshot( + val version: Int, + val modified: Instant, + val content: String, + ) - var content: String = content - internal set + /* + * One volatile reference, so a reader can never pair a new version with the old content. The editor + * dispatches change events from a background coroutine per edit, so two edits in one frame do reach + * this concurrently. + */ + @Volatile + private var snapshot = Snapshot(version, modified, content) - fun inputStream(): BufferedInputStream { - return content.byteInputStream().buffered() - } + val version: Int + get() = snapshot.version + + val modified: Instant + get() = snapshot.modified + + val content: String + get() = snapshot.content - fun reader(): BufferedReader { - return content.reader().buffered() + /** + * Publishes [content] at [version], or returns false if [version] is older than what is already + * published. + * + * A version that moves backwards makes the Kotlin index mint a second `KtFile` for text that never + * changed, which is what surfaced as redeclaration errors across a whole file (ADFA-5231). + */ + internal fun update( + version: Int, + content: String, + ): Boolean { + synchronized(this) { + if (version < snapshot.version) return false + snapshot = Snapshot(version, Instant.now(), content) + return true + } } + + fun inputStream(): BufferedInputStream = content.byteInputStream().buffered() + + fun reader(): BufferedReader = content.reader().buffered() } diff --git a/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt new file mode 100644 index 0000000000..b6efed4f33 --- /dev/null +++ b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt @@ -0,0 +1,47 @@ +package com.itsaky.androidide.projects + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.models.Range +import org.junit.After +import org.junit.Test +import java.nio.file.Paths + +/** A document's version and content always move forward together. */ +class ActiveDocumentVersionTest { + private val path = Paths.get("/tmp/adfa5231/Main.kt") + + @After + fun close() { + FileManager.onDocumentClose(DocumentCloseEvent(path)) + } + + private fun change( + text: String, + version: Int, + ) = DocumentChangeEvent(path, text, text, version, ChangeType.NEW_TEXT, 0, Range.NONE) + + @Test + fun `a backwards version is rejected and leaves the newer content in place`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v3", 3)) + + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version).isEqualTo(3) + assertThat(document.content).isEqualTo("v3") + } + + @Test + fun `a version and its content are never observed apart`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version to document.content).isEqualTo(2 to "v2") + } +} From 46e492f6f2db5d166fe3daaa9cb499bd873e58d3 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 25 Aug 2026 13:47:50 +0000 Subject: [PATCH 2/2] ADFA-5231: document the fileVersion reset race and public API of ActiveDocument The editor-reuse reset paths (release(), dispatchDocumentOpenEvent()) stamp fileVersion outside documentChangeMutex, so a reset can race an in-flight increment from a still-running change dispatch. This is a pre-existing, bounded exposure distinct from the same-document backwards-version bug this ticket fixes - record it instead of silently accepting it. Also document the public ActiveDocument properties and update()'s equal-version behavior. --- .../java/com/itsaky/androidide/editor/ui/IDEEditor.kt | 9 +++++++++ .../itsaky/androidide/projects/models/ActiveDocument.kt | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt index 918d69af7e..e7d65d9a08 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt @@ -144,6 +144,15 @@ open class IDEEditor private var actionsMenu: EditorActionsMenu? = null private var _signatureHelpWindow: SignatureHelpWindow? = null private var _diagnosticWindow: DiagnosticWindow? = null + + /** + * [documentChangeMutex] only serialises change dispatches against each other; the resets in + * [release] and [dispatchDocumentOpenEvent] run outside it, so a reset can race an in-flight + * [dispatchDocumentChangeEvent]'s `incrementAndGet()` and stamp a low version right after a + * newly-opened file's counter is zeroed. This is tolerated: it is bounded (self-heals on the + * next edit) and distinct from the same-document backwards-version bug this ticket fixes, + * which `ActiveDocument.update` now guards regardless of how `fileVersion` got there. + */ private val fileVersion = AtomicInteger(0) private val documentChangeMutex = Mutex() internal var isModified = false diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt index 89bdeab453..09b28ce6f6 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt @@ -47,12 +47,15 @@ open class ActiveDocument( @Volatile private var snapshot = Snapshot(version, modified, content) + /** The version last published via [update]. Always consistent with [content] and [modified]. */ val version: Int get() = snapshot.version + /** The timestamp of the last [update]. Always consistent with [version] and [content]. */ val modified: Instant get() = snapshot.modified + /** The content last published via [update]. Always consistent with [version] and [modified]. */ val content: String get() = snapshot.content @@ -62,6 +65,11 @@ open class ActiveDocument( * * A version that moves backwards makes the Kotlin index mint a second `KtFile` for text that never * changed, which is what surfaced as redeclaration errors across a whole file (ADFA-5231). + * + * An equal version is accepted and overwrites, rather than being rejected like an older one. The + * only writer, `IDEEditor`, stamps versions from a single serialised `AtomicInteger.incrementAndGet()` + * per document, so distinct edits never share a version - an equal version is a re-delivery of the + * same edit, and taking its (identical) content is harmless. */ internal fun update( version: Int,