-
Notifications
You must be signed in to change notification settings - Fork 4
fix(t1): the registry stops serving rows it cannot observe (#480 #481 #482 #468) #486
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
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. 🟡 Medium cmuxlayer/src/agent-registry.ts Line 1956 in 4e04bfa
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ import { | |
| import { validateSurfaceIdentityBijection } from "./surface-topology.js"; | ||
| import { deriveCmuxObserverOwnerId } from "./cmux-observer-identity.js"; | ||
| import { inferRepoFromDirectory } from "./repo-workspace.js"; | ||
| import { resumeArtifactStatus } from "./resume-verification.js"; | ||
|
|
||
| export type SurfaceProvider = () => Promise<CmuxSurface[]>; | ||
|
|
||
|
|
@@ -105,6 +106,23 @@ export function deriveSurfaceObserverId( | |
|
|
||
| export const SURFACE_EVICTION_CONFIRMATION_MS = 5_000; | ||
|
|
||
| /** | ||
| * Absence window for rows NO live observer claims (#480). | ||
| * | ||
| * `canMutateForObservedAbsence` requires an exact observer match, so a row | ||
| * whose `surface_observer_id` is null (pre-observer-identity) or belongs to a | ||
| * dead socket generation could never be evicted, never be crash-marked, and | ||
| * never be purged: worst-case survival was unbounded. Measured 2026-08-19: | ||
| * four such rows, oldest 36 days, `list_agents` 17 vs `list_surfaces` 13. | ||
| * | ||
| * Ownership exists to stop one observer mutating another's LIVE row. It is not | ||
| * a claim on a row that no live surface bears — matched on the row's UUID when | ||
| * it has one, else on its ref — across a continuous absence window. This constant is that window: twelve consecutive | ||
| * 5 s sweeps of proven absence before an unclaimed row is dropped, so the | ||
| * worst case is bounded and documented rather than infinite. | ||
| */ | ||
| export const UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS = 60_000; | ||
|
|
||
| export interface AgentFilter { | ||
| state?: AgentState; | ||
| repo?: string; | ||
|
|
@@ -501,6 +519,16 @@ export class AgentRegistry { | |
| string, | ||
| { surfaceId: string; firstObservedAt: number } | ||
| >(); | ||
| /** | ||
| * Absence clock for rows this observer does not own (#480). Kept separate | ||
| * from `surfacelessObservations` on purpose: that map is cleared by the | ||
| * ownership gate itself (`isSurfacelessConfirmed`), so an unclaimed row can | ||
| * never accumulate time in it. | ||
| */ | ||
| private unclaimedAbsenceObservations = new Map< | ||
| string, | ||
| { surfaceId: string; firstObservedAt: number } | ||
| >(); | ||
| private stateMgr: StateManager; | ||
| private surfaceProvider: SurfaceProvider; | ||
| private observerId: string | null; | ||
|
|
@@ -645,7 +673,7 @@ export class AgentRegistry { | |
| async reconstitute(opts: SurfaceAbsenceOptions = {}): Promise<Set<string>> { | ||
| this.agents.clear(); | ||
| this.aliases.clear(); | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
|
|
||
| const stateFiles = this.stateMgr.listStates(); | ||
| for (const record of stateFiles) { | ||
|
|
@@ -749,7 +777,7 @@ export class AgentRegistry { | |
| // Incomplete or contradictory identity evidence can prove neither | ||
| // presence nor absence. | ||
| // Reset pending absence timers so a later valid scan starts fresh. | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| return new Set(); | ||
| } | ||
| const liveSurfaceKeys = this.liveSurfaceKeys(surfaces); | ||
|
|
@@ -909,6 +937,7 @@ export class AgentRegistry { | |
| const aliases = this.aliasesResolvingTo(resolved); | ||
| this.agents.delete(resolved); | ||
| this.surfacelessObservations.delete(resolved); | ||
| this.unclaimedAbsenceObservations.delete(resolved); | ||
| this.aliases.delete(agentId); | ||
| this.aliases.delete(resolved); | ||
| for (const alias of aliases) { | ||
|
|
@@ -1013,7 +1042,7 @@ export class AgentRegistry { | |
| if (!discoveryIsBijective || discoveryHasMixedIdentity) { | ||
| // A degraded discovery scan must break any pending negative-evidence | ||
| // streak even when exact UUID matches remain safe for positive sync. | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| } | ||
| if (!discoveryIsBijective) { | ||
| return this.list(opts?.filter).map((record) => ({ | ||
|
|
@@ -1237,7 +1266,7 @@ export class AgentRegistry { | |
| const discoveryHasMixedIdentity = | ||
| hasMixedDiscoveryIdentityCoverage(discovered); | ||
| if (!discoveryIsBijective || discoveryHasMixedIdentity) { | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| } | ||
| if (!discoveryIsBijective) { | ||
| return opts?.agentId ? this.get(opts.agentId) : null; | ||
|
|
@@ -1629,6 +1658,12 @@ export class AgentRegistry { | |
| if (surfacelessObservation?.surfaceId === this.agentSurfaceKey(record)) { | ||
| this.surfacelessObservations.set(newAgentId, surfacelessObservation); | ||
| } | ||
| const unclaimedObservation = | ||
| this.unclaimedAbsenceObservations.get(oldAgentId); | ||
| this.unclaimedAbsenceObservations.delete(oldAgentId); | ||
| if (unclaimedObservation?.surfaceId === this.agentSurfaceKey(record)) { | ||
| this.unclaimedAbsenceObservations.set(newAgentId, unclaimedObservation); | ||
| } | ||
| for (const [alias, target] of this.aliases) { | ||
| if (target === oldAgentId) { | ||
| this.aliases.set(alias, newAgentId); | ||
|
|
@@ -1693,7 +1728,7 @@ export class AgentRegistry { | |
| return []; | ||
| } | ||
| if (!hasCoherentSurfaceIdentity(surfaces)) { | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| return []; | ||
| } | ||
|
|
||
|
|
@@ -1704,10 +1739,40 @@ export class AgentRegistry { | |
| for (const [id, agent] of [...this.agents.entries()]) { | ||
| if (agent.transcript_session_capture_deferred === true) { | ||
| this.surfacelessObservations.delete(agent.agent_id); | ||
| this.unclaimedAbsenceObservations.delete(agent.agent_id); | ||
| continue; | ||
| } | ||
| if (this.matchingLiveSurface(agent, surfaces)) { | ||
| this.surfacelessObservations.delete(agent.agent_id); | ||
| this.unclaimedAbsenceObservations.delete(agent.agent_id); | ||
| continue; | ||
| } | ||
| // #480: rows no live observer claims take the bounded unclaimed path | ||
| // instead of dying at the ownership gate below. They cannot be evicted, | ||
| // crash-marked (`reconcileSurfaces` applies the same gate) or recovered | ||
| // (`recoverCrashedAgents` quarantines unowned rows) on any other path, | ||
| // so without this they live forever. | ||
| if (!this.canMutateForObservedAbsence(agent, observerSnapshot.ownerId)) { | ||
|
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. 🟠 High
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: |
||
| // The row is the only `agent_id` -> `cli_session_id` mapping, and | ||
| // `resumeAgent` is the one recovery path with no ownership gate. So a | ||
| // row whose captured session is still on disk is not a ghost: it is | ||
| // the record resume-by-ID acts on, and a successful resume re-stamps | ||
| // it with this observer. Deleting it would strand a live transcript. | ||
| // Only a PRESENT artifact retains: `missing` restores nothing, and | ||
| // `unverifiable` (no store on this machine) must not make eviction | ||
| // depend on a directory's existence -- that would reopen #480 wherever | ||
| // the harness store is absent. | ||
| if (this.hasVerifiedResumeArtifact(agent)) { | ||
| this.unclaimedAbsenceObservations.delete(agent.agent_id); | ||
| continue; | ||
| } | ||
| if (!this.isUnclaimedAbsenceConfirmed(agent, opts)) { | ||
| continue; | ||
| } | ||
| const removedUnclaimedId = this.evictUnchecked(id); | ||
| if (removedUnclaimedId) { | ||
| evicted.push(removedUnclaimedId); | ||
| } | ||
| continue; | ||
| } | ||
| if (!this.isSurfaceAbsenceAuthoritative(agent, surfaces)) { | ||
|
|
@@ -1874,6 +1939,52 @@ export class AgentRegistry { | |
| return now - observation.firstObservedAt >= confirmationMs; | ||
| } | ||
|
|
||
| /** | ||
| * #480/#482: a captured session this machine can still see on disk — the | ||
| * one thing an unclaimed row still protects, since `resumeAgent` has no | ||
| * ownership gate and the registry holds the only agent_id -> session map. | ||
| */ | ||
| private hasVerifiedResumeArtifact(agent: AgentRecord): boolean { | ||
| if (!agent.cli_session_id) return false; | ||
| return resumeArtifactStatus(agent.cli, agent.cli_session_id) === "present"; | ||
| } | ||
|
|
||
| /** | ||
| * Continuous absence of a row that this observer does not own (#480). | ||
| * | ||
| * The row's identity is ONE key: its UUID when it has one, else its ref | ||
| * (`agentSurfaceKey`). Absence means the caller's `matchingLiveSurface` | ||
| * found nothing for that key in a coherent, non-empty scan. | ||
| * | ||
| * Deliberately does NOT consult `isSurfaceAbsenceAuthoritative`: that helper | ||
| * refuses to read a UUID-less row's absence in a UUID-bearing topology, | ||
| * because a live occupant sitting ON the same mutable ref proves nothing | ||
| * about the row. Here the ref is not occupied at all. That is real absence | ||
| * evidence, and it is the only evidence an unclaimed row can ever produce. | ||
| * | ||
| * Eviction, not crash-marking: dropping the registry row is the reversible | ||
| * direction. If the pane were somehow alive, `listMerged` re-mints it from | ||
| * discovery on the next call; marking a live agent `error` would not | ||
| * self-correct. | ||
| */ | ||
| private isUnclaimedAbsenceConfirmed( | ||
| agent: AgentRecord, | ||
| opts: { now?: number }, | ||
| ): boolean { | ||
| const surfaceKey = this.agentSurfaceKey(agent); | ||
| const confirmationMs = UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS; | ||
| const now = opts.now ?? Date.now(); | ||
| const observation = this.unclaimedAbsenceObservations.get(agent.agent_id); | ||
| if (!observation || observation.surfaceId !== surfaceKey) { | ||
| this.unclaimedAbsenceObservations.set(agent.agent_id, { | ||
| surfaceId: surfaceKey, | ||
| firstObservedAt: now, | ||
| }); | ||
| return false; | ||
| } | ||
| return now - observation.firstObservedAt >= confirmationMs; | ||
| } | ||
|
|
||
| private canMutateForObservedAbsence( | ||
| agent: AgentRecord, | ||
| observerEpoch?: string | null, | ||
|
|
@@ -1895,6 +2006,11 @@ export class AgentRegistry { | |
| return !owner || Boolean(observerId && owner === observerId); | ||
| } | ||
|
|
||
| private clearAbsenceObservations(): void { | ||
| this.surfacelessObservations.clear(); | ||
| this.unclaimedAbsenceObservations.clear(); | ||
| } | ||
|
|
||
| private clearSurfacelessObservationsForLiveSurfaces( | ||
| liveSurfaceKeys: ReadonlySet<string>, | ||
| ): void { | ||
|
|
@@ -1903,6 +2019,11 @@ export class AgentRegistry { | |
| this.surfacelessObservations.delete(agentId); | ||
| } | ||
| } | ||
| for (const [agentId, observation] of this.unclaimedAbsenceObservations) { | ||
| if (liveSurfaceKeys.has(observation.surfaceId)) { | ||
| this.unclaimedAbsenceObservations.delete(agentId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| repairFromDiscovery( | ||
|
|
@@ -1916,7 +2037,7 @@ export class AgentRegistry { | |
| !hasBijectiveDiscoveryIdentity(discovered) || | ||
| hasMixedDiscoveryIdentityCoverage(discovered) | ||
| ) { | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| return { repaired: [], evicted: [], skipped: [] }; | ||
| } | ||
| const repaired: RegistryRepairEntry[] = []; | ||
|
|
@@ -2463,7 +2584,7 @@ export class AgentRegistry { | |
| return 0; | ||
| } | ||
| if (!hasCoherentSurfaceIdentity(surfaces)) { | ||
| this.surfacelessObservations.clear(); | ||
| this.clearAbsenceObservations(); | ||
| return 0; | ||
| } | ||
| const liveSurfaceKeys = this.liveSurfaceKeys(surfaces); | ||
|
|
||
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.
🟢 Low
cmuxlayer/README.md
Line 165 in 64a16cf
The README labels the mutating tool table as
Mutating (29), but removingresync_agentsleaves only 28 listed tools, so the documented count is incorrect. Update the heading toMutating (28)and adjust any aggregate counts that are intended to describe the listed tools.🚀 Reply "fix it for me" or copy this AI Prompt for your agent: