mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 00:49:41 +02:00
fix(sessions): make the discard a real inverse of the construction
Third review of the reboot-restore branch. The narrow discard the previous commit introduced avoided everything cleanupSession() did wrongly, and in dropping so much of it also dropped four things it had to keep. The worst broke the retry the whole design rests on. setupSessionListeners() returns early while sessionListenerRefs still holds the session id, and the discard never cleared that entry. So the advertised flow — a rebuild fails because the agent binary is missing, the user fixes their PATH and clicks again — reused the same id, wired no listeners at all, and produced a tab that never showed output, never updated its status and never persisted. That is worse than the leak the discard was added to prevent. Three more registrations leaked with it: a RunSummaryTracker and its interval, an image watcher on the workspace, and the Ralph fix-plan watcher. The discard now undoes each registration setupSessionListeners() makes, in its order, and the per-session custom-model config directory, which holds the endpoint's API key literally and which nothing else would ever remove. The image-watcher flag was restored after the code that reads it, so a session came back reporting the feature as on with nothing watching. It moves to the before-spawn phase, and that phase now runs before the listeners rather than after them. The generation counter that lets a mid-restore dismiss win was global while clear() is ownership-scoped, so one user's dismiss discarded another user's unspent entries, permanently, because nothing rebuilds an in-memory plan. It is now per owner. Bumping only the owners of entries the dismiss removed was not enough either: take() has already emptied the plan by then, so a dismiss landing mid-restore saw nothing of that owner's to remove and invalidated nothing. The owners that matter are those with a restore in flight, filtered by what the dismissing user may access, and that is what clear() now bumps. Plan expiry bumps too, so a restore straddling the 24-hour boundary cannot hand entries back and give an expired plan another full day. Tests. discardPartiallyBuiltSession had no test at all: the only implementation any test ran was the mock's one-line stub, which is why every defect above was invisible. test/discard-partially-built-session.ts drives the real WebServer, and the retry assertion fails if the listener refs are left behind — verified by reverting the fix. The dismiss-race test drove the registry by hand, so deleting the route's generation argument left it green; it now goes through the route, and two further tests cover the multi-user cases. The mock context has now gone stale twice, because route tests pass it as `ctx as never` and tsconfig.json includes only src, so nothing ever compares it to the ports. A type-level guard is therefore inert — I wrote one and confirmed it never fires. test/mocks/mock-route-context-completeness.ts compares the mock's keys against WebServer.createRouteContext() at runtime instead, and names what is missing. Also: the API reference now says workspace-forbidden is judged against the owner's grant, the banner's module header no longer claims Restore always dismisses it, and the detail span gets the same min-width: 0 the phone rule already needed. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fa52753e8b
commit
71ed7b127c
@@ -48,12 +48,16 @@ export class RebootRestoreRegistry {
|
||||
/** When the boot pass built the plan, in ms since the epoch. */
|
||||
private builtAt = 0;
|
||||
/**
|
||||
* Bumped by anything that invalidates entries a restore is already holding.
|
||||
* A Dismiss arriving mid-restore must win: without this the route's `finally`
|
||||
* would put its unspent entries back and resurrect the offer the user just
|
||||
* cleared, with a fresh 24-hour life.
|
||||
* Per owner, bumped by anything that invalidates that owner's entries while a
|
||||
* restore is already holding them. A Dismiss arriving mid-restore must win:
|
||||
* without this the route's `finally` would put its unspent entries back and
|
||||
* resurrect the offer the user just cleared, with a fresh 24-hour life.
|
||||
*
|
||||
* Keyed by owner rather than global, because `clear()` is ownership-scoped. A
|
||||
* single counter would let one user's Dismiss discard another user's unspent
|
||||
* entries, and the plan is in-memory, so those offers would be gone for good.
|
||||
*/
|
||||
private generation = 0;
|
||||
private generations = new Map<string | undefined, number>();
|
||||
/**
|
||||
* Owners with a restore in flight, between its take and its last pane.
|
||||
* Keyed by owner so one user's restore does not turn another user's click into
|
||||
@@ -66,12 +70,17 @@ export class RebootRestoreRegistry {
|
||||
set(entries: readonly RebootRestoreEntry[]): void {
|
||||
this.entries = new Map(entries.map((entry) => [entry.sessionId, entry]));
|
||||
this.builtAt = entries.length > 0 ? Date.now() : 0;
|
||||
this.generation += 1;
|
||||
this.bumpAll();
|
||||
}
|
||||
|
||||
/** The current generation, for a caller that will later return entries. */
|
||||
currentGeneration(): number {
|
||||
return this.generation;
|
||||
/**
|
||||
* The generations of the owners of `entries`, for a caller that will hand some
|
||||
* of them back later. Pass the result to {@link restore}.
|
||||
*/
|
||||
snapshotGenerations(entries: readonly RebootRestoreEntry[]): Map<string | undefined, number> {
|
||||
const snapshot = new Map<string | undefined, number>();
|
||||
for (const entry of entries) snapshot.set(entry.owner, this.generations.get(entry.owner) ?? 0);
|
||||
return snapshot;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -117,12 +126,20 @@ export class RebootRestoreRegistry {
|
||||
* hand is NOT put back, because that one cannot stop being true, and an entry
|
||||
* the banner keeps re-offering forever is noise only Dismiss can clear.
|
||||
*/
|
||||
restore(entries: readonly RebootRestoreEntry[], generation?: number): void {
|
||||
// A dismiss (or a fresh boot plan) since the caller took these entries means
|
||||
// they are no longer wanted back.
|
||||
if (generation !== undefined && generation !== this.generation) return;
|
||||
for (const entry of entries) this.entries.set(entry.sessionId, entry);
|
||||
if (entries.length > 0 && this.builtAt === 0) this.builtAt = Date.now();
|
||||
restore(entries: readonly RebootRestoreEntry[], generations?: ReadonlyMap<string | undefined, number>): void {
|
||||
let added = 0;
|
||||
for (const entry of entries) {
|
||||
// A dismiss (or a fresh boot plan) for THIS entry's owner since the caller
|
||||
// took it means it is no longer wanted back. Another owner's dismiss is
|
||||
// none of this entry's business.
|
||||
if (generations) {
|
||||
const taken = generations.get(entry.owner);
|
||||
if (taken !== undefined && taken !== (this.generations.get(entry.owner) ?? 0)) continue;
|
||||
}
|
||||
this.entries.set(entry.sessionId, entry);
|
||||
added += 1;
|
||||
}
|
||||
if (added > 0 && this.builtAt === 0) this.builtAt = Date.now();
|
||||
}
|
||||
|
||||
/** Drop the entries a viewer can see. Returns how many went. */
|
||||
@@ -130,8 +147,14 @@ export class RebootRestoreRegistry {
|
||||
const removable = [...this.entries.values()].filter((entry) => canAccess(entry.owner));
|
||||
for (const entry of removable) this.entries.delete(entry.sessionId);
|
||||
if (this.entries.size === 0) this.builtAt = 0;
|
||||
// Any restore currently in flight must not put its entries back afterwards.
|
||||
this.generation += 1;
|
||||
// A restore in flight for these owners must not put their entries back. The
|
||||
// in-flight owners are the ones that matter and the ones the plan can no
|
||||
// longer name: `take()` has already removed their entries, so a dismiss that
|
||||
// lands mid-restore sees nothing of theirs to remove. The bump is limited to
|
||||
// owners this caller could see, so it cannot reach anyone else's restore.
|
||||
const invalidated = new Set(removable.map((entry) => entry.owner));
|
||||
for (const owner of this.spending) if (canAccess(owner)) invalidated.add(owner);
|
||||
for (const owner of invalidated) this.bump(owner);
|
||||
return removable.length;
|
||||
}
|
||||
|
||||
@@ -155,11 +178,25 @@ export class RebootRestoreRegistry {
|
||||
this.entries.clear();
|
||||
this.builtAt = 0;
|
||||
this.spending.clear();
|
||||
this.generation += 1;
|
||||
this.generations.clear();
|
||||
}
|
||||
|
||||
private bump(owner: string | undefined): void {
|
||||
this.generations.set(owner, (this.generations.get(owner) ?? 0) + 1);
|
||||
}
|
||||
|
||||
/** Invalidate every owner's in-flight returns, including owners not yet seen. */
|
||||
private bumpAll(): void {
|
||||
for (const owner of new Set([...this.entries.values()].map((entry) => entry.owner))) this.bump(owner);
|
||||
for (const owner of [...this.generations.keys()]) this.bump(owner);
|
||||
}
|
||||
|
||||
private dropIfExpired(): void {
|
||||
if (this.builtAt > 0 && Date.now() - this.builtAt > PLAN_TTL_MS) {
|
||||
// Bump before clearing, while the owners are still known: a restore that
|
||||
// took entries just before the expiry must not hand them back afterwards
|
||||
// and give an expired plan another full day of life.
|
||||
this.bumpAll();
|
||||
this.entries.clear();
|
||||
this.builtAt = 0;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user