Repository navigation
fix(agent): activate a dormant graph's saved row when StorageACK requests arrive for it - #3902
branarakic-agent wants to merge 1 commit into
Conversation
A core declined StorageACKs for a graph whose saved subscription row a restart left dormant, until its paced backlog reached the row. A row saved before its graph had an on-chain id was checked with a name lookup through the finalized authority index, which waits out the index's catch-up after a restart; the row then fell to background recovery, which a request did not hurry. When the gate finds the namespace's row dormant for a reason that clears on its own, it now checks and activates that row at once, with the on-chain id the request names (already checked against the graph's committed name) as its binding: the same read-authority check and activation transaction as the paced owners, without their pause, slot limit or name lookup. One check per row and two at a time, at most 32 rows, checked again while requests keep coming for up to ten minutes. The graphs ACK requests reached recently (16 at most, for a day) are kept in the data directory, and startup rehydration brings their rows back first and outside the activation cap.
| && ports.subscription(id) === subscription | ||
| ); | ||
| try { | ||
| await ports.activate(fresh!, onChainId, isCurrentActivation); |
There was a problem hiding this comment.
🔴 Bug: Concurrent rolling recovery can leave demand activation falsely committed
What's wrong
The new demand path can install a provisional subscribed row while the rolling worker is checking the same graph. The rolling worker treats those provisional flags as completed activation and clears dormancy. This cancels the activation transaction, whose rollback then skips cleanup because ownership was retired. Consumers subsequently see an active subscription whose setup never completed.
Example
With rolling capacity available, pause demand activation during responsibility reconciliation. Let the rolling worker finish checking the same row. It clears dormancy, causing demand activation to fail while leaving the provisional subscription installed without completing sync and gossip setup.
Suggested direction
Share activation ownership across workers, and require committed activation before the rolling worker clears dormancy for an existing live row.
For Agents
Coordinate storage-ack-demand-activation.ts with the rolling worker in context-graph-subscription-authority-recovery.ts and the activation owner. Add a controlled overlap test proving that a provisional row either completes activation or rolls back without being reported as committed.
| expect(verdict).toEqual({ ok: true }); | ||
| // The rest of the backlog stays where the budget and the cap left it. | ||
| const reasons = [...after.state.contextGraphSubscriptionDormancyById.values()]; | ||
| expect(reasons.filter((reason) => reason === 'authorityUnavailable').length).toBe(HOSTED_ROWS + 4); |
There was a problem hiding this comment.
🔴 Bug: The restart test gives false confidence about the new cap exemption
What's wrong
The test claims to verify restoration outside the cap, but its target is already core-hosted, which was exempt before this PR. Its dormancy-count assertions therefore cannot detect removal of the new recent-row cap exemption.
Example
Changing countsAgainstCap back to !row.coreHosted would leave this test green: TARGET becomes coreHosted before the second restart. With cap=1, a recent non-hosted row and an ordinary authorized row should both activate; that regression would activate only the recent row.
Suggested direction
Add a restart case using a recent non-hosted subscription and enough authority budget to exercise successful activation accounting.
For Agents
Extend storage-ack-dormant-restart.test.ts with a persisted recent entry whose subscription remains non-hosted. Assert that it activates without consuming the ordinary subscription's cap slot.
| return; | ||
| } | ||
| const writing = this.#writeLoop().finally(() => { | ||
| if (this.#writing === writing) this.#writing = undefined; |
There was a problem hiding this comment.
🟡 Issue: A request arriving as the writer retires can lose its persisted priority
What's wrong
There is a microtask window where #writeLoop has finished but #writing remains set. A new note only sets #writeAgain; no loop remains to consume it. The graph stays absent from disk, and repeated requests within the refresh hour do not schedule another write, so its restart priority can be lost.
Example
Schedule note('second', '2') between the write loop's final condition check and its finally callback. After whenWritten() resolves, memory contains both graphs but the persisted JSON contains only the first. This interleaving reproduced with injected asynchronous I/O.
Suggested direction
Recheck pending writes during retirement, or make checking for pending work and releasing writer ownership atomic.
For Agents
Repair writer retirement in storage-ack-recent-graphs.ts. Preserve coalescing while ensuring a note delivered during retirement triggers another write. Verify that whenWritten() leaves both requested graphs persisted under this interleaving.
| } | ||
| } | ||
|
|
||
| async #attempt(demand: Demand, reason: DemandReason): Promise<AttemptOutcome> { |
There was a problem hiding this comment.
🟡 Issue: Share the per-row promotion protocol instead of adding another implementation
What's wrong
The new scheduler also implements the subscription promotion protocol already owned by rolling activation. Different pacing and binding inputs justify a separate scheduling policy, but duplicating the row-validation and activation sequence creates another place where the same lifecycle invariants must be maintained.
Example
The new sameRow helper compares subscription intent and binding fields, while rolling promotion maintains its own inline comparisons and intent checks. A future change to subscription eligibility or stale-row fencing must now update these implementations separately.
Suggested direction
Keep scheduling policies separate, but move durable-row loading, authority resolution, fresh-row validation, and activation fencing into one subscription-owned operation with explicit outcomes. Let the demand scheduler supply the verified binding and decide when to retry.
For Agents
Extract the common per-row authority-and-activation operation from context-graph-subscription-authority-recovery.ts and use it from rolling and demand activation. Preserve demand-specific binding, deadlines, concurrency, retry timing, and cap exemption. Run the existing demand, rolling-promotion, and dormant-restart tests.
| contextGraphId: string, | ||
| onChainId: string, | ||
| ): boolean { | ||
| const agent = agentObject as DemandActivationAgent; |
There was a problem hiding this comment.
🟡 Issue: Keep the agent adapter compiler-checked
What's wrong
The public object parameter erases the dependency contract before asserting a substantial parallel definition of the agent. This bypasses the useful typed ports already defined above and hides coupling to subscription state, lifetime ownership, persistence, and reconciliation from the compiler.
Example
requestStorageAckDemandActivation({}, 'graph', '1') type-checks despite lacking every required dependency. Similarly, renaming an agent field does not connect the actual agent type to the hand-maintained DemandActivationAgent declaration.
Suggested direction
Wire StorageAckDemandActivationPorts explicitly where the agent's protected state is accessible, then retain the resulting owner on the agent or through a typed registration boundary. The recent-graphs helper can likewise receive its small typed configuration directly.
For Agents
Construct the demand activation ports at the agent boundary, following the existing rolling-promotion adapter. Remove the object-to-DemandActivationAgent assertion and apply a checked boundary to the recent-graphs adapter as well. Preserve ownership across lifetime changes and verify with type-checking and the existing restart tests.
| onChainId: row.onChainId ?? demand.onChainId, | ||
| ...(row.onChainHash === undefined ? {} : { onChainHash: row.onChainHash }), | ||
| }; | ||
| const checkSignal = AbortSignal.any([ports.signal, AbortSignal.timeout(this.#checkTimeoutMs)]); |
There was a problem hiding this comment.
🟡 Issue: In-flight authority deadlines and shutdown cancellation lack coverage
What's wrong
The test resolver ignores the supplied signal. The shutdown test returns UNAVAILABLE immediately and aborts during retry backoff; the fairness test also returns immediately. Neither verifies cancellation of an in-flight check, which matters because two stalled reads can occupy every demand-activation slot.
Example
Hold an authority read until its supplied signal aborts, queue another graph with maxConcurrent=1, and expire checkTimeoutMs. Assert that the queued graph activates. Separately abort the node while the read is pending and assert that whenIdle settles without activation.
Suggested direction
Exercise pending authority reads that settle only upon cancellation, covering the check deadline and node shutdown.
For Agents
Add in-flight cancellation cases in storage-ack-demand-activation.test.ts. Make resolveAuthority observe its signal and prove both timeout-driven slot release and shutdown-driven settlement.
Summary
What we saw
A testnet core with about 1,500 saved subscription rows and a small activation cap restarted four times in an hour. Each start left almost every row dormant: about 1,270 behind the cap, and about 260 hosted rows deferred past the 10 s startup authority budget. A publisher wrote to a graph created that morning, and its row on this core was one of those behind the cap:
activationCap)chain-name-binding-unavailable); row moved toauthorityUnavailableIn the last round the publisher had only one other fast ACK, so its publish waited for this core: quorum came 15 s after the request round started.
Two things combined:
With an ACK quorum of 2 and only a few cores, a core that declines a graph for the life of each worker stalls publishing to it whenever another core is unavailable.
Changes
storage-ack-demand-activation.ts(new). When the gate finds the namespace's saved row dormant for a reason that clears on its own (activationCap,authorityUnavailable), it hands the row to a demand activation instead of only waiting. The demand activation reads the row's authority with the request's on-chain id as the binding when the saved row has none; current policy and roster are still read live, as for any saved row. It activates the row through the same transaction and fences the paced owners use, without their pause, their slot limit or the name lookup.storage-ack-recent-graphs.ts(new). The graphs ACK requests reached recently, kept instorage-ack-recent-graphs.jsonin the data directory: 16 at most, dropped after a day without requests, written only when the set changes or a saved time is an hour old (temp file, then rename). It only orders work: a missing or unreadable file costs the ordering and nothing else.How long it takes now
A restarted core with 1,533 saved rows, activation cap 4, the startup budget spent at once, and a name lookup that never answers (the index still walking):
On a node the check is one live liveness and policy read (two chain reads with authority priority) plus the activation's store writes, so we expect the publisher's first or second retry, 1 to 3 s after its first request, to be signed, during an index catch-up as well. In the last testnet round above that would have been 2 s instead of 12 s. The core logs
Activated persisted context-graph subscription "<id>" for StorageACK requests <N> ms after the first, andRestoring <n> recently ACK-requested context-graph subscription(s) firstat start.Relation to #3901
#3901 keeps the authority index from walking its history again after a restart; it changes only
packages/chain, and this change onlypackages/agent, so they do not overlap. They are independent: with #3901 the index caught up within a minute of the latest restart, and with this change the ACK path no longer waits for it either way.Tests
storage-ack-demand-activation.test.ts(fake timers): activation at once with the request's binding; a hosted row recovery holds; checks again after 1, 2, 4 s while authority is unavailable; one check per row and two at a time; a failing row does not hold up the next graph; the row limit; a denial is final; a row saved under another id is left alone; stops when requests stop and after 10 minutes; stops when the node stops; checks again when the row changes under the check; leaves a row another owner activated.storage-ack-recent-graphs.test.ts: kept across a restart; a busy graph is written again only hourly; bounded and aged out; an unreadable file is ignored and replaced; a request noted before the file is read keeps the saved list.storage-ack-dormant-restart.test.ts: the three restart cases above, on a real agent with a mock chain.context-graph-dormant-core-host-admission.test.ts: a capped retained row is now declined once, activated for the request with its member intent and reconcile watermark kept, and admitted on the next request. A row with rehydration disabled is still declined finally and left untouched.Not changed