docs(components): document app cleanup on shutdown via scope 'close' - #647
docs(components): document app cleanup on shutdown via scope 'close'#647Ethan-Arrowood wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds documentation for application shutdown cleanup and updates the Plugin API reference to detail how scope.close() and 'close' event listeners handle asynchronous cleanup in version 5.1.3. The review feedback suggests style guide corrections to use hyphens instead of em dashes as field separators, and recommends adding a <VersionBadge> component to highlight the behavior change in scope.close().
| #### Events | ||
|
|
||
| - **`'close'`** — Emitted after `scope.close()` is called | ||
| - **`'close'`** — Emitted after `scope.close()` is called. Harper calls `scope.close()` itself on shutdown and on graceful restart, so this is the hook for application teardown — see [Cleanup on Shutdown](#cleanup-on-shutdown) |
There was a problem hiding this comment.
According to the general rules, hyphens (-) should be used instead of em dashes (—) as field separators in Markdown documentation (e.g., separating configuration option names from their descriptions). Please update the 'close' event description to use a hyphen.
| - **`'close'`** — Emitted after `scope.close()` is called. Harper calls `scope.close()` itself on shutdown and on graceful restart, so this is the hook for application teardown — see [Cleanup on Shutdown](#cleanup-on-shutdown) | |
| - **`'close'`** - Emitted after `scope.close()` is called. Harper calls `scope.close()` itself on shutdown and on graceful restart, so this is the hook for application teardown — see [Cleanup on Shutdown](#cleanup-on-shutdown) |
References
- In Markdown documentation, use hyphens ('-') instead of em dashes ('—') as field separators (e.g., separating configuration option names from their descriptions) to adhere to the style guide, even if the file has pre-existing inconsistencies using em dashes.
There was a problem hiding this comment.
Not taking this one. The em-dash rule is being over-generalized here.
AGENTS.md says em dashes "are fine for parenthetical asides and match existing prose; do not use them as field separators." The field-separator prohibition refers to the bullet immediately above it - Type: / Default: reference blocks, where each field goes on its own line. It is not a ban on - **term** — description in prose lists.
Two checks: the docs tree has 633 em-dash list-item lines across 63 reference files, and AGENTS.md itself uses this exact pattern - including in the rule being quoted (- **Hyphens vs. dashes** — em dashes...).
So these are the established convention, not "pre-existing inconsistencies." Applying the suggestion would leave the touched lines inconsistent with every sibling line in the same list.
sent with Claude Opus 5
| #### `scope.close()` | ||
|
|
||
| Closes all associated entry handlers and the `scope.options` instance, emits `'close'`, and removes all listeners. | ||
| Closes all associated entry handlers and the `scope.options` instance, emits `'close'`, and removes all listeners. Promises returned by `'close'` listeners are awaited before it resolves. |
There was a problem hiding this comment.
When documenting behavior changes to an existing API surface, we should use the component for standalone placement after headings. Since the behavior of scope.close() awaiting 'close' listener promises was introduced in v5.1.3, let's add a right after the heading.
| #### `scope.close()` | |
| Closes all associated entry handlers and the `scope.options` instance, emits `'close'`, and removes all listeners. | |
| Closes all associated entry handlers and the `scope.options` instance, emits `'close'`, and removes all listeners. Promises returned by `'close'` listeners are awaited before it resolves. | |
| #### `scope.close()` | |
| <VersionBadge type="changed" version="v5.1.3" /> | |
| Closes all associated entry handlers and the `scope.options` instance, emits `'close'`, and removes all listeners. Promises returned by `'close'` listeners are awaited before it resolves. |
References
- When documenting behavior changes to an existing API surface (e.g., adding new fields to a response), use .
- Reserve the component for standalone placement after headings. For inline version annotations mid-sentence or inside list items, use plain text (e.g.,
(vX.Y.Z)) instead, as using the component mid-sentence is awkward.
|
|
||
| - **v4.6.0** — Plugin API introduced (experimental) | ||
| - **v4.7.0** — Further improvements to the Plugin API | ||
| - **v5.1.3** — Shutdown awaits promises returned by `'close'` listeners; earlier releases emitted `'close'` without waiting, so async cleanup could be cut off by the worker exiting |
There was a problem hiding this comment.
According to the general rules, hyphens (-) should be used instead of em dashes (—) as field separators in Markdown documentation. Please update the v5.1.3 version history entry to use a hyphen.
| - **v5.1.3** — Shutdown awaits promises returned by `'close'` listeners; earlier releases emitted `'close'` without waiting, so async cleanup could be cut off by the worker exiting | |
| - **v5.1.3** - Shutdown awaits promises returned by `'close'` listeners; earlier releases emitted `'close'` without waiting, so async cleanup could be cut off by the worker exiting |
References
- In Markdown documentation, use hyphens ('-') instead of em dashes ('—') as field separators (e.g., separating configuration option names from their descriptions) to adhere to the style guide, even if the file has pre-existing inconsistencies using em dashes.
There was a problem hiding this comment.
Not taking this one. The em-dash rule is being over-generalized here.
AGENTS.md says em dashes "are fine for parenthetical asides and match existing prose; do not use them as field separators." The field-separator prohibition refers to the bullet immediately above it - Type: / Default: reference blocks, where each field goes on its own line. It is not a ban on - **term** — description in prose lists.
Two checks: the docs tree has 633 em-dash list-item lines across 63 reference files, and AGENTS.md itself uses this exact pattern - including in the rule being quoted (- **Hyphens vs. dashes** — em dashes...).
So these are the established convention, not "pre-existing inconsistencies." Applying the suggestion would leave the touched lines inconsistent with every sibling line in the same list.
sent with Claude Opus 5
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-647 This preview will update automatically when you push new commits. |
The only coverage of shutdown cleanup was a single line in the Scope
events list ("Emitted after `scope.close()` is called"), which reads as
if the plugin author calls `scope.close()` themselves. Nothing stated
that Harper calls it during shutdown and graceful restart, which is the
reason an application would listen for the event at all.
- Add a "Cleanup on Shutdown" section to plugin-api.md with the
`scope.once('close', ...)` pattern, covering the shutdown sequence:
async listeners are awaited, cleanup is bounded by the termination
backstop, a rejecting listener is logged and stops the wait on its
siblings, and each worker cleans up independently.
- Clarify the `'close'` event and `scope.close()` entries in place and
link them to the new section.
- Add a "Shutdown Cleanup" pointer to applications.md so the
application-building audience finds it (that page had no occurrence of
"cleanup", "shutdown", or "teardown").
- Record the v5.1.3 behavior change in the Version History list.
Verified against harper: componentLoader.ts:700 calls `scope.close()` on
the SHUTDOWN ITC message, restartWorkers posts the same message, and
Scope.ts:275-288 awaits promises returned by 'close' listeners.
Closes #604
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on #647: scope.close() awaits promises returned by 'close' listeners as of v5.1.3 (v5.1.2 emitted 'close' without awaiting). Per CONTRIBUTING.md, behavior changes to existing surface get a standalone <VersionBadge type="changed" /> below the heading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fb9d522 to
f0818d8
Compare
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-647 This preview will update automatically when you push new commits. |
Summary
Shutdown cleanup for applications was technically documented but undiscoverable: the entire coverage was one line in the
Scopeevents list inplugin-api.md— "Emitted afterscope.close()is called" — which reads as if the plugin author callsscope.close()themselves. Nothing said Harper calls it, which is the whole reason an application would listen for the event.reference/components/plugin-api.md— new## Cleanup on Shutdownsection with thescope.once('close', ...)pattern, stating that Harper callsscope.close()on shutdown and on graceful restart. Clarified the'close'event bullet and thescope.close()entry in place, and linked both to the new section.reference/components/applications.md— new## Shutdown Cleanuppointer cross-linking to it. That page previously had zero occurrences of "cleanup", "shutdown", or "teardown", so the application-building audience had nothing to find.Source evidence
Verified in
HarperFast/harperatv5.2.6(latest release) before writing:scope.close()on shutdown —components/componentLoader.ts:700:restartWorkersposts the sameSHUTDOWNITC message (server/threads/manageThreads.js:555-559), reached frombin/restart.ts:187(restartoperation) andmanageThreads.js:1218(dev watch reload).'close'listeners are awaited —components/Scope.ts:275-288collectsthis.listeners('close')andawait Promise.all(...)over their return values.server/threads/threadServer.js:205-211:.then(() => closeServers()).then(() => whenScopesClosed()).then(() => realExit(0)), backed by thecomponents/scopeShutdown.tsregistry (which logs a rejected close and treats it as settled, so failed cleanup can't wedge shutdown).threadTerminationTimeout(10s; 30s underDEV_MODE) inmanageThreads.js:87-105, with the main thread force-terminating at 2x (manageThreads.js:585).Status of HarperFast/harper#1912
Still open, but the code it describes has already been fixed and released — so this PR documents the corrected behavior rather than the caveat the issue asked for.
harper#1912 cites
Scope.ts:181-187(barethis.emit('close')),componentLoader.ts:472(fire-and-forgetscope.close()), andthreadServer.js:175-177(closeServers().then(() => realExit(0))). That is the pre-fix layout. Both halves of its "Suggested direction" landed in commitd72d5b0("fix: dispose component scopes before worker exit in dev, and serialize reloads", harper#1323).Bisecting the release tags for the awaited-listener code in
components/Scope.ts:v5.0.0-v5.1.2emit('close')— async cleanup racedprocess.exitv5.1.3-v5.2.6'close'listener promisesSo the docs state that async cleanup is awaited, with the remaining genuine caveat being the termination backstop. harper#1912 looks stale and is probably closable — flagging rather than acting on it here.
Notes (not addressed in this PR)
npm run buildreports two pre-existing broken anchors in files this PR does not touch:/reference/v5/backups/overview->/reference/v5/cli/commands#backing-up-with-volume-snapshots/release-notes/v5-lincoln/5.1->#deployment-operationsAlso, the repo's committed
node_modulesstate was missing@harperfast/code-guidelines, soformat:checkfails in a stale checkout untilnpm ciis re-run.Verification
npm run format:check— cleannpm run build— succeeds; all anchors added by this PR validateCloses #604
🤖 Generated with Claude Code