Repository navigation
Files others write are read by one owner on one serialized path - #114
Conversation
docs/SYSTEMS.md, sections 1 and 2, with the rule agreed after PR 1: each state owner updates its state through one serialized path and publishes immutable snapshots; external changes are rechecked before writes. - Workers: the shared threads, a bounded pool for file work and one timer that only hands work on. - Strand: an owner's tasks one at a time and in order, on those threads. - FileWatch: the one watcher of the folders Companion follows. A folder that does not exist yet is watched through its nearest existing ancestor, one removed while watched is watched again, and one that cannot be watched is read on retries after 1 and 5 seconds and every 30 seconds after. Events are filtered by name. - FileReading: the owner of a value read from such a file. Every read and every change of the value runs on its strand, whoever asks; a write is read once it settled, a failed read is tried again, and changed() fires only when a read found another value. - KeyAssignments is a FileReading of options.txt; its own watcher, scheduler and counters go, and with them the races the last reviews found between a page's read and its own. The order of the remaining PRs puts the files before the remaining owners. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e33ea1a57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.pending != null) this.pending.cancel(false); | ||
| }); |
There was a problem hiding this comment.
If a reader is already running when close() is called, this task is queued behind that read, so closed remains false and readHere() can still update last and fire changed() after closure was requested. This is reproducible with a blocking reader: call readNow(), wait until the reader starts, call close(), then release the reader; the subscriber is still notified. Mark closure synchronously with a thread-safe flag before enqueueing the watcher and timer cleanup so an in-flight read cannot publish after close() returns.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| WatchKey key = folder.register(this.service, StandardWatchEventKinds.ENTRY_CREATE, | ||
| StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.ENTRY_DELETE); | ||
| this.keys.put(folder, key); | ||
| this.folders.put(key, folder); |
There was a problem hiding this comment.
When two normalized path spellings identify the same directory, such as a real path and a symlink to it, WatchService.register() can return the same WatchKey for both registrations. This assignment then overwrites the first key-to-folder mapping, so run() filters out the first follower and it no longer receives changes; closing either registration can also cancel the shared key while the other remains. Preserve every logical path associated with a key, or canonicalize existing ancestors before registration.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| Path at = followed.folder; | ||
| while (at != null && !Files.isDirectory(at)) at = at.getParent(); | ||
| followed.at = at != null && register(at) ? at : null; |
There was a problem hiding this comment.
If the missing target folder is created after this existence scan has moved at to an ancestor but before that ancestor is registered, its creation event occurs before the watch starts. The registration then succeeds on the ancestor, so no retry is scheduled and the follower can remain there permanently; the initial read may see the new file, but later writes inside the target folder are never observed because directory watches are not recursive. Recheck the target after registering an ancestor, repeating placement until the observed directory state is covered by the active watch.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
A folder made between looking for the nearest existing one and watching it, as a game makes its folders at once on its first start, was never seen, and the watch stayed a level too high. Once a folder is watched, the one beneath is looked for again, down to the folder followed; ancestors passed on the way are let go. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…shares a watch by folder From the review of the slice: - A write seen while a read runs is counted at once, on the watcher's thread, and that read publishes nothing, so a file read half written is never shown; the read after the write settled is. - close() stops publication at once, so a read under way then tells nothing. - A folder reached through a link shares its watch with every other path to it, and each follower is matched by its watch, so one closing does not stop another. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5e2c24da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| now = this.reader.read(this.file); | ||
| } catch (IOException | RuntimeException failure) { | ||
| this.unreadable = true; |
There was a problem hiding this comment.
When a file event increments generation while an older read is running and that older read then fails, this assignment still marks the current reading unreadable. If the latest read succeeds with the same value already published, readHere() consequently fires changed() solely because of the obsolete failure, violating the signal's change-only contract and causing unnecessary downstream reloads. Only record the failure when this read's generation is still current, as the previous implementation did.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| } | ||
| List<Runnable> tell = new ArrayList<>(); | ||
| synchronized (this) { | ||
| boolean gone = !key.reset(); |
There was a problem hiding this comment.
On Linux, renaming the followed directory generally leaves its WatchKey valid and attached to the moved inode, so this reset() returns true and place() is never called. Because the parent is not watched while the target directory exists, recreating the original path and writing the followed file produces no event; the reading remains attached to the renamed-away directory indefinitely. Watch the parent or otherwise verify that the key still represents the requested path before treating it as active.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| public void moveTo(Path file) { | ||
| Path followed = Objects.requireNonNull(file, "file").toAbsolutePath().normalize(); | ||
| this.strand.execute(() -> follow(followed)); |
There was a problem hiding this comment.
If the old file's reader is still running when moveTo() is called, this method does not advance generation until the queued follow() eventually runs. The old read can therefore publish and fire changed() after the move was requested, briefly exposing state from a world or other file that is no longer current before the new file is read. Invalidate the active generation synchronously when requesting the move, as readNow(), written(), and close() already invalidate their in-flight work.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
…ertaken read changes nothing From the reviews of the slice: - Owners' strands run on threads of their own, so a page's read on the file work that waits for an owner's first value never holds the thread that value needs. - A follower whose callback fails is logged, and the watch goes on for every other. - A read another write overtook marks nothing unreadable, so the next read with the same value tells nobody. - moveTo() makes a read of the file before give way at once, as readNow(), a write and close() do. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 301fe46b4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.strand.execute(() -> { | ||
| try { | ||
| Read<T> now = this.last; | ||
| read.complete(now != null ? now.value() : readHere(0)); |
There was a problem hiding this comment.
When the initial owner read has failed and left last null, value() starts another read with generation 0. If the watcher reports a new write while that read is blocked, written() advances the generation, but readHere() explicitly exempts generation 0 from the mismatch check, so it publishes a potentially partial, obsolete value and fires changed() due to the prior failure before the settled read runs. Fresh evidence beyond the earlier superseded-read comments is that the current first-value branch still hardcodes this bypass; make this read participate in generation supersession and complete it from the newest read instead.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| private T readHere(long generation) throws IOException { | ||
| T now; | ||
| try { | ||
| now = this.reader.read(this.file); |
There was a problem hiding this comment.
readHere() is invoked by a strand backed by the four-thread OWNERS pool, so this blocking file access never uses Workers.files(). Once four readings block on slow or unavailable files, every other owner's strand—including follow, retry, and close tasks—stops making progress, and file-worker callers waiting in value() also remain blocked. Dispatch the blocking reader to the file-work pool and return its result to the strand, as required by the shared systems architecture.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| } else if (lost || names.stream().anyMatch(followed.names)) { | ||
| tell.add(followed.changed); |
There was a problem hiding this comment.
If any registration's names predicate throws a runtime exception while processing an event, the exception escapes anyMatch() and the outer loop, whose catch handles only interruption and service closure. This permanently terminates the application's sole watch thread, so unrelated FileReading instances silently stop receiving changes; evaluate each follower predicate with the same failure isolation already used for changed callbacks below.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
1 participant
Third PR of
docs/SYSTEMS.md, stacked on #113: files others write are read by one owner on one serialized path. It brings the rule agreed after PR 1:Finish line
FileReadingTestunless named)aWriteInPartsIsReadOnceItSettledAndAnEqualOneTellsNothingaValueAskedBeforeTheOwnersFirstReadIsWhatTheNextChangeIsToldAgainst;KeyAssignmentsTest.aPageThatReadBeforeTheOwnerStillHearsOfTheNextChangeaFolderThatAppearsLaterIsFollowedOnceItDoes;KeyAssignmentsTest.aGameFolderThatAppearsLaterIsWatchedOnceItDoesaFailedReadIsTriedAgainAndToldWhenItSucceedsreadsNeverRunAtOnceaReadingMovedToAnotherFileReadsItAndTellsaClosedReadingTellsNothingoptions.txtCatalogPanelsTest.keyBindingsKeepTheirSelectionWhenTheirKeysAreReadAgain,PageReadsTestWhat changes
Workers: the shared threads, a bounded pool of platform threads for file work and one timer that only hands work on.Strand: an owner's tasks one at a time and in order, on those threads, instead of a thread per owner.FileWatch: the one watcher of the folders Companion follows. A folder that does not exist yet is watched through its nearest existing ancestor; one removed while watched is watched again; one that cannot be watched at all is read on retries after 1 and 5 seconds and every 30 seconds after. Events are filtered by name.FileReading: the owner of a value read from such a file. Every read and every change of the value runs on its strand, whoever asks; a write is read once it settled; a failed read is tried again;changed()fires only when a read found another value.KeyAssignmentsis aFileReadingofoptions.txt, 45 lines instead of 186. Its own watcher, scheduler and counters go, and with them the class of races the last reviews found between a page's read and its own.SystemsRulesTestlistsWorkersandFileWatchas the system and dropsKeyAssignments' exceptions.docs/SYSTEMS.md: the rule,FileReadingas built, and the order of the remaining PRs, which puts the files before the remaining owners.Production code: +451 −185, the mechanisms arriving; the next PRs move the other file followers onto them.
:companion:testpasses (1624).Left as it is
🤖 Generated with Claude Code