-
Notifications
You must be signed in to change notification settings - Fork 542
fix(runtime,desktop): order same-revision catalog reads by a run epoch #5741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eef38a3
f5850bc
7bb7e57
305d157
c0b34f2
4603526
8ad0764
b76062d
2efb158
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -188,9 +188,39 @@ export function waitForCatalogSession( | |
| }); | ||
| } | ||
|
|
||
| /** A committed row at a newer revision is authoritative over an older snapshot of it. */ | ||
| /** | ||
| * A committed row at a newer revision is authoritative over an older snapshot | ||
| * of it. Equal revisions tie on the live run state's own order: a turn | ||
| * starting or ending does not move `revision`, so two same-revision reads can | ||
| * disagree about `runningTurnIds` — the run epoch says which observation is | ||
| * older, and the stale one must not overwrite the fresher (#5713). | ||
| * | ||
| * The epoch counter only orders observations of one Host generation. | ||
| * Generations themselves are not ordered, so a read from a different | ||
| * generation is never stale: a restarted Host must take the row over from its | ||
| * predecessor whatever the two counters read (#5713 review). A successful | ||
| * cross-generation response cannot exist on the wire, either: closing a | ||
| * connection rejects every in-flight request with `connection_lost` | ||
| * (client/connection.ts), so a lagging predecessor read never delivers after | ||
| * the successor's row has landed. | ||
| */ | ||
| function isStaleSummary(prior: DesktopSessionSummary, next: DesktopSessionSummary): boolean { | ||
| return prior.revision > next.revision; | ||
| if (prior.revision !== next.revision) return prior.revision > next.revision; | ||
| const priorGeneration = prior.runHostGeneration; | ||
| const nextGeneration = next.runHostGeneration; | ||
| if ( | ||
| priorGeneration !== undefined && | ||
| nextGeneration !== undefined && | ||
| priorGeneration !== nextGeneration | ||
| ) { | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: A different random generation is treated as newer in both directions. If a read from Host A is delayed, Host B restarts and its same-revision row is committed, then A's already-issued response arrives, this branch returns |
||
| } | ||
| const priorEpoch = prior.runEpoch; | ||
| const nextEpoch = next.runEpoch; | ||
| if (priorEpoch === undefined || nextEpoch === undefined || priorEpoch === nextEpoch) { | ||
| return false; | ||
| } | ||
| return priorEpoch > nextEpoch; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: |
||
| } | ||
|
|
||
| export const selectSessions = (state: SessionCatalogState): readonly DesktopSessionSummary[] => | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 — This holds for authoritative Host reads, but not for every row that reaches the catalog. After a restart,
session-local-service.tscatalog()can still return the predecessor's rows aslocalState: 'cached'(runningTurnIds stripped, butrunEpoch/runHostGenerationkept) until the new Host's catalog refresh lands. Since cross-generation rows are never stale, such a row can briefly overwrite the successor's row. That matches the old last-writer-wins behaviour, so it's not a regression. Suggest narrowing the wording to "an authoritative (non-cached) predecessor read never delivers late", and optionally not emittingrunEpoch/runHostGenerationon cached rows.