Repository navigation
fix(feed-debug): keep a reloaded session's entries, and fail closed for real - #1111
Merged
Merged
Conversation
…or real Two bugs in one persistence path, both found by auditing the open-issue table against main. `feedDebugLog.ts` had no test file at all, which is how the second one survived its own comment saying the opposite. **#771 — the branch written to preserve entries was the branch that lost them.** When `stat` fails with anything but ENOENT the writer refuses to append on an unknown baseline, and its comment says it returns "WITHOUT advancing the cursor so the renderer resends these entries". It `return`ed — which RESOLVES the IPC, and the renderer advances its durable cursor in `.then` and retains only in `.catch`. So every entry in that batch was marked persisted and dropped. It now throws, and the renderer's existing retry path (already covered by "retries rejected rows on the next tick") does the rest. **#770 — a soft reload made a session stop persisting for the rest of the run.** Reload rebuilds the runtime, so `feedDebugNextId` restarts at 1 and the epoch is minted fresh. Three cursors survived that: the renderer's persisted and in-flight refs, which live outside the runtime on purpose so a busy agent does not re-render the workspace on every batch, and main's process-local de-dup cursor. With the renderer's at 4,812 every new entry is filtered as already-sent; with main's, every id is dropped as already-written. The trail vanished exactly when someone was reloading BECAUSE the feed had gone wrong. The renderer refs are cleared at the reload site. Main's cannot be — an explicit reset message would race an append already in flight — so the batch now carries the generation that numbered it and main resets the id cursor when that changes. Cap state is deliberately NOT reset: the file is the same file and its byte count is still true, so a reload must not re-open a capped log. The clear is scoped to the arm that actually restarts the ids. Soft reload's no-durable-transcript arm is deliberately non-destructive — it keeps the runtime — so clearing there would make the renderer re-send everything it had already persisted. A test pins that distinction so the fix cannot degrade into "delete the cursors on anything called soft reload". Six mutations measured one at a time, all killed: fail-closed returns instead of throwing, epoch change does not reset, epoch resets unconditionally, the reload does not clear, it clears only the in-flight ref, and the batch stops carrying the epoch. `npx tsc -b` clean on Node 24; main + renderer, unit + renderer: 3919 passed. Closes #770 Closes #771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eopening #770 An append from the old id generation can still be in flight when a soft reload deletes both cursors. When it settled afterwards, its .then wrote the OLD generation's max id into the persisted cursor, and every new entry at or below it was filtered as already written: #770's silent loss, back by timing. Both settle handlers now check the batch's epoch against the session's current one and do nothing for a stale generation. The new test fails first on the real hook (cursor 3, expected 2) and drives the reload exactly as forgetFeedDebugCursors runs it. Found by a cross-agent read-only review of the PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cts review found untested Review round 1 on #1111 (one Codex reviewer), all valid: - Major: the epoch was the first entry's timestamp, so a reset inside the same millisecond (or after a backwards clock step) reused the old generation's epoch. Main then kept the old cursor and dropped the new ids, and the settle guard let a stale write move the new cursor. Epochs are now minted as max(now, last + 1) per renderer process: still a real millisecond timestamp for tMs, but never repeated. tMs is clamped at 0. - The catch-side generation guard, the IPC's epoch forwarding and cap preservation across a reload were each removable with every test green. Each now has a test that fails under exactly that mutation (verified): a stale REJECTION keeping the new reservation, the debug:append-feed-log handler forwarding epoch and refusal, and a real 128 MiB sparse file staying capped across an epoch change. - The renderer collision test builds both generations in one fake millisecond; the first version let fake time separate them, so it could not fail. Caught by running the mutation, not by reading. Deferred as #1207: per-session maps resurrected by an append queued before forget. It predates this PR and applies to all three maps. Co-Authored-By: Claude Opus 5.5 <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.
Closes #770, #771. Both found by auditing the open-issue table against main.
src/main/storage/feedDebugLog.tshad no test file at all, which is how #771 survived while its own comment described the opposite behaviour.#771 — the branch written to preserve entries was the branch that discarded them
When
statfails with anything other than ENOENT, the writer refuses to append on an unknown baseline. Its comment:It
returned — which resolves the IPC. The renderer advances its durable cursor in.thenand retains only in.catch, so every entry in that batch was marked persisted and thrown away. The one branch that exists to protect entries was the one losing them.It now throws. The renderer's retry path already existed and is already covered ("retries rejected rows on the next tick"), which is what makes a one-word change the complete fix rather than half of one.
#770 — a soft reload made a session stop persisting for the rest of the run
Reload rebuilds the runtime, so
feedDebugNextIdrestarts at 1 andfeedDebugEpochMsis minted fresh on the next entry. Three cursors survived that:persistedFeedDebugIdRefandinFlightFeedDebugIdRef— which live outside the runtime on purpose, so a busy agent does not re-render the workspace on every persisted batch, and which therefore nothing in the reset reaches;lastWrittenFeedDebugId.With the renderer's at 4,812,
selectFeedDebugAppendBatchfilters every new entry asid <= lastPersistedIdand sends nothing, ever again. If anything did get through, main's cursor drops it as already-written. The diagnostic trail disappeared at exactly the moment someone was reloading because the feed had gone wrong.The renderer refs are cleared at the reload site. Main's cannot be — an explicit "reset" message would race an append already in flight — so the batch now carries the generation that numbered it, and main resets its id cursor when that changes. The epoch travels with the data, so it cannot get out of order with it.
Two deliberate non-resets, both tested:
Version skew degrades to the old behaviour rather than a new one: an absent
epochMsfrom an older renderer is treated as "same generation", which is what main did before.Verification
Six mutations, each applied individually against a verified-clean tree, all killed:
returns instead of throwingnpx tsc -bclean on Node 24.src/main+src/renderer, unit + renderer projects: 3919 passed, 0 failed.The three existing payload assertions in
useFeedDebugPersist.renderer.test.tsxwere updated to nameepochMsexplicitly rather than relaxed totoMatchObject— a payload that quietly stops carrying the generation is precisely the bug.🤖 Generated with Claude Code
Review (two rounds, one Codex reviewer): MERGE-READY
31bc9188, fail-first test1ae9b60e(strictly increasing mint); 4 tests fail without it1ae9b60e)tMsreads 0 until wall time catches uptsis intact