feat(tui): H3 — SingleFlightCache<K,V> + Published<T>, typed ADR 0008 invariants (#46)#59
Open
jinyeow wants to merge 2 commits into
Open
feat(tui): H3 — SingleFlightCache<K,V> + Published<T>, typed ADR 0008 invariants (#46)#59jinyeow wants to merge 2 commits into
jinyeow wants to merge 2 commits into
Conversation
…f the lock Dispose now acts as the final supersede: both post-fetch guards check _disposed, so a token-ignoring fetch that completes (or faults) after Dispose() can neither publish nor throw out of its returned task. Two new TCS-driven tests pin both paths (each confirmed failing first). CTS Cancel()/Dispose() now run outside _gate in ScheduleAsync and Dispose: Cancel() executes token registrations synchronously, and running arbitrary callbacks under the lock is a lock-inversion hazard for any consumer whose registration takes another lock. Under the lock we only detach the old CTS and bump the stamp/_disposed, so each detached CTS is cancelled exactly once and correctness still rests on the stamp, not the cancel. No new test for this: a deterministic RED for a cross-thread lock inversion would be a hanging test, worse than none — the existing cancellation tests pin the observable behaviour. Also documents the publish-callback constraint: it runs under the cache's internal lock, so it must stay a cheap reference swap.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await Assert.ThrowsAsync<ObjectDisposedException>( | ||
| () => cache.ScheduleAsync("b", (_, _) => Task.FromResult(2), Publish(published))); | ||
|
|
||
| cache.Dispose(); // double-dispose is a no-op |
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
2 participants
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 #46. Track 2 foundation (tracking issue #36), H3 — the last-but-one foundation ticket before the workspace build.
What
Types the ADR 0008 load invariants so the flagship list+preview workspace (#48/#49) builds on types instead of re-copied conventions (the four PR #11 bugs came from exactly that shape):
SingleFlightCache<TKey, TValue>(src/Cobalt.Tui/Tasks/SingleFlightCache.cs) — the supersede primitive generalizing the_loadSeqstamp-guard pattern fromPrListViewModel/WorkItemListViewModel: owns its oneCancellationTokenSource(linked to a lifetime token); scheduling a new key cancels and abandons the previous fetch; only the newest may publish, gated by the monotonic supersede stamp checked atomically with any concurrent schedule (cancellation is cooperative — the stamp, not the cancel, is the correctness guarantee). A superseded fetch's fault is observed and swallowed so it can never reach theUnobservedTaskExceptioncrash-log hook (ADR 0013); the newest fetch's fault propagates out of its returned task. Deliberately not the diff dialog's join/dedup cache (that shares one fetch between converging callers and stays where it is).Published<T>(src/Cobalt.Tui/Tasks/Published.cs) — the atomic single-reference publish/read pair generalizingPrDiffViewModel.DiffState: the pair that must never tear lives inside one immutable record, published by a single volatile write and snapshotted by a single volatile read.The two primitives are orthogonal; the composition test drives the ticket's acceptance verbatim (fake source with controllable latency, move A→B→C fast → only C publishes, A/B cancelled, no torn snapshot, no unobserved fault).
Per the ticket's owned-files list, no existing view-model is migrated — the workspace is the first consumer; migration is follow-up.
Review
Own review + Codex cross-model pass (pre-authorized), findings applied on-branch:
Dispose()didn't invalidate the stamp — a token-ignoring fetch completing after dispose could still publish (or throw). Both completion guards now treat dispose as the final supersede; two RED-first tests pin it.Cancel()ran token registrations synchronously under the internal lock (lock-inversion hazard). CTS is now detached under the lock and cancelled/disposed outside it. No new test — a deterministic cross-thread lock-inversion test is a hanging test; existing cancellation tests pin the observable behaviour.ScheduleAsync: publish must stay a cheap reference swap (e.g.Published<T>.Publish), never raise events or take locks.Tests
12 new tests (
tests/Cobalt.Tui.Tests/Tasks/), TCS-driven, no timing races: supersede + token cancellation, stamp guard vs a token-ignoring fetch, superseded-fault observed / newest-fault propagates, lifetime cancel, dispose semantics (incl. publish/fault after dispose), stamp monotonicity,Published<T>set/clear, and the A→B→C composition. Full suite green; clean CI-style Release build (--no-incremental,ContinuousIntegrationBuild=true) with 0 warnings.