fix(publish): bounded grace for the post-confirm tail — publish response no longer gated on store-queue congestion (#1572) #1590
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /** | ||
| * Bounded grace for post-confirm publish "tails" (GH #1572). | ||
| * | ||
| * After a V10 publish confirms on-chain, the caller-facing result | ||
| * (ual / kaId / txHash / status) is complete — but the code still runs a | ||
| * serial tail of store mutations (SWM cleanup sweeps, `_meta` lifecycle | ||
| * stamps). Every one of those mutations is submitted to the store queue | ||
| * with no deadline, so on a node whose store queue is congested (large | ||
| * accumulated graphs + sustained inbound ACK/SWM traffic) the tail — and | ||
| * therefore the HTTP response that awaits it — can take many minutes. | ||
| * Measured on mainnet cores: confirm at +4s, response at +9..18 min and | ||
| * growing, with the TCP connection dying at ~12 min. The same publish on | ||
| * an idle node completes the tail in well under a second. | ||
| * | ||
| * `awaitTailWithGrace` keeps the tail's semantics intact while unhooking | ||
| * it from the response path: | ||
| * - the tail always runs to completion, in order, exactly once; | ||
| * - if it settles within `graceMs`, behavior is byte-identical to a plain | ||
| * `await` (including error propagation to the caller); | ||
| * - if it is still running after `graceMs`, the caller proceeds (the | ||
| * response goes out) and the tail continues detached; its eventual | ||
| * settlement is reported through `onDetachedSettled` so a failure is | ||
| * logged rather than becoming an unhandled rejection. | ||
| * | ||
| * On a healthy node the grace never elapses, so this changes nothing. | ||
| */ | ||
|
|
||
| export const DEFAULT_PUBLISH_TAIL_GRACE_MS = 5_000; | ||
|
|
||
| /** | ||
| * Resolve the grace budget from `DKG_PUBLISH_TAIL_GRACE_MS`. Invalid or | ||
| * missing values fall back to {@link DEFAULT_PUBLISH_TAIL_GRACE_MS}; `0` | ||
| * is honored (respond immediately, tail fully detached). | ||
| */ | ||
| export function resolvePublishTailGraceMs(env: NodeJS.ProcessEnv = process.env): number { | ||
| const raw = env.DKG_PUBLISH_TAIL_GRACE_MS; | ||
| if (raw === undefined || raw === '') return DEFAULT_PUBLISH_TAIL_GRACE_MS; | ||
| const n = Number(raw); | ||
| if (!Number.isFinite(n) || n < 0) return DEFAULT_PUBLISH_TAIL_GRACE_MS; | ||
| return Math.floor(n); | ||
| } | ||
|
|
||
| const TAIL_GRACE_TIMEOUT: unique symbol = Symbol('publish-tail-grace-timeout'); | ||
|
|
||
| /** | ||
| * Await `tail` for up to `graceMs`. | ||
| * | ||
| * Returns `'completed'` when the tail settled in time — a rejection inside | ||
| * the grace window is rethrown to the caller, exactly like a plain `await`. | ||
| * Returns `'detached'` when the grace elapsed first; the still-running tail | ||
| * keeps executing and `onDetachedSettled` fires exactly once when it | ||
| * eventually settles (with the error when it failed, `undefined` on | ||
| * success), guaranteeing the rejection is consumed. | ||
| */ | ||
| export async function awaitTailWithGrace( | ||
|
There was a problem hiding this comment. 🟡 Issue: The tail-grace abstraction is too low-level, so the policy is still scattered across call sites What's wrong Example Suggested direction For Agents |
||
| graceMs: number, | ||
| tail: Promise<void>, | ||
| onDetachedSettled: (error?: unknown) => void, | ||
| ): Promise<'completed' | 'detached'> { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| const winner = await Promise.race([ | ||
| tail, | ||
| new Promise<typeof TAIL_GRACE_TIMEOUT>((resolve) => { | ||
| timer = setTimeout(() => resolve(TAIL_GRACE_TIMEOUT), Math.max(0, graceMs)); | ||
| // Never keep the process alive just to time out a grace window. | ||
| timer.unref?.(); | ||
| }), | ||
| ]); | ||
| if (winner === TAIL_GRACE_TIMEOUT) { | ||
| tail.then( | ||
| () => onDetachedSettled(), | ||
| (err) => onDetachedSettled(err), | ||
| ); | ||
| return 'detached'; | ||
| } | ||
| return 'completed'; | ||
| } finally { | ||
| if (timer !== undefined) clearTimeout(timer); | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { | ||
| awaitTailWithGrace, | ||
| resolvePublishTailGraceMs, | ||
| DEFAULT_PUBLISH_TAIL_GRACE_MS, | ||
| } from '../src/publish-tail-grace.js'; | ||
|
|
||
| const sleep = (ms: number) => new Promise<void>((r) => setTimeout(r, ms)); | ||
|
|
||
| describe('awaitTailWithGrace (GH #1572)', () => { | ||
| it('returns "completed" when the tail settles inside the grace, without firing the detach callback', async () => { | ||
| let detached = 0; | ||
| const outcome = await awaitTailWithGrace(1_000, sleep(20), () => { detached += 1; }); | ||
| expect(outcome).toBe('completed'); | ||
| // give any stray callback a chance to fire before asserting | ||
| await sleep(50); | ||
| expect(detached).toBe(0); | ||
| }); | ||
|
|
||
| it('rethrows a tail failure that happens inside the grace (plain-await parity)', async () => { | ||
| let detached = 0; | ||
| const failing = (async () => { await sleep(10); throw new Error('store exploded'); })(); | ||
| await expect(awaitTailWithGrace(1_000, failing, () => { detached += 1; })).rejects.toThrow('store exploded'); | ||
| await sleep(50); | ||
| expect(detached).toBe(0); | ||
| }); | ||
|
|
||
| it('returns "detached" promptly when the tail outlives the grace, and the tail still completes', async () => { | ||
| let tailDone = false; | ||
| let detachedError: unknown = 'not-called'; | ||
| const tail = (async () => { await sleep(300); tailDone = true; })(); | ||
| const started = Date.now(); | ||
| const outcome = await awaitTailWithGrace(50, tail, (err) => { detachedError = err; }); | ||
| const waited = Date.now() - started; | ||
| expect(outcome).toBe('detached'); | ||
| expect(waited).toBeLessThan(250); // responded at ~graceMs, not at tail completion | ||
| expect(tailDone).toBe(false); // tail genuinely still running at detach time | ||
| await sleep(400); | ||
| expect(tailDone).toBe(true); // ...and it ran to completion afterwards | ||
| expect(detachedError).toBeUndefined(); // settled-ok callback fired with no error | ||
| }); | ||
|
|
||
| it('routes a post-grace tail failure into the callback instead of an unhandled rejection', async () => { | ||
| let detachedError: unknown; | ||
| const tail = (async () => { await sleep(150); throw new Error('late failure'); })(); | ||
| const outcome = await awaitTailWithGrace(20, tail, (err) => { detachedError = err; }); | ||
| expect(outcome).toBe('detached'); | ||
| await sleep(300); | ||
| expect(detachedError).toBeInstanceOf(Error); | ||
| expect((detachedError as Error).message).toBe('late failure'); | ||
| }); | ||
|
|
||
| it('graceMs=0 detaches immediately for a pending tail', async () => { | ||
| let tailDone = false; | ||
| const tail = (async () => { await sleep(100); tailDone = true; })(); | ||
| const outcome = await awaitTailWithGrace(0, tail, () => {}); | ||
| expect(outcome).toBe('detached'); | ||
| expect(tailDone).toBe(false); | ||
| await sleep(200); | ||
| expect(tailDone).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('resolvePublishTailGraceMs', () => { | ||
| it('defaults when unset, empty, or invalid; honors valid values including 0', () => { | ||
| expect(resolvePublishTailGraceMs({} as NodeJS.ProcessEnv)).toBe(DEFAULT_PUBLISH_TAIL_GRACE_MS); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: '' } as NodeJS.ProcessEnv)).toBe(DEFAULT_PUBLISH_TAIL_GRACE_MS); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: 'soon' } as NodeJS.ProcessEnv)).toBe(DEFAULT_PUBLISH_TAIL_GRACE_MS); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: '-5' } as NodeJS.ProcessEnv)).toBe(DEFAULT_PUBLISH_TAIL_GRACE_MS); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: '0' } as NodeJS.ProcessEnv)).toBe(0); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: '12000' } as NodeJS.ProcessEnv)).toBe(12_000); | ||
| expect(resolvePublishTailGraceMs({ DKG_PUBLISH_TAIL_GRACE_MS: '2500.9' } as NodeJS.ProcessEnv)).toBe(2_500); | ||
| }); | ||
| }); |
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
Oops, something went wrong.
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.
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.
🟡 Issue: The detachable metadata tail should be a named operation, not a 140-line inline async closure
What's wrong
The PR identifies a real conceptual boundary: everything after this point is post-response metadata tail work. But instead of making that boundary part of the structure, it wraps a large, already dense block in an immediately invoked async function inside a 5k-line file. That makes the publish flow harder to scan and makes future edits riskier because the detached work's scope is defined by distant braces rather than a named operation.
Example
A reader has to scan from
const metaTail = (async () => {through all VM pointer stamps, lifecycle flips, publishedUal writes, assertion graph updates, andswmShareCompletecleanup before reaching the line that actually applies the grace. The new async boundary is not visible from a named function or indentation structure.Suggested direction
Use the new tail boundary as the decomposition point. Move the metadata bookkeeping into a focused
writePublishMetadataTail/runPostPublishMetadataTailhelper that receives the few values it needs, and keep the main publish flow as: compute result, run metadata tail with grace, return result. That deletes the hidden closure and makes the detachable work explicit without changing behavior.For Agents
Extract the block currently wrapped by
metaTailinpackages/agent/src/dkg-agent-publish.tsinto a named private method or local pure orchestration function such aswritePublishMetadataTail(...). Preserve all existing internal best-effort logging and the final return shape. Then call the publish-tail grace runner around that named work. Existing publish/finalize tests should still cover the metadata rows; add a narrow test if extracting changes the observable sequencing.