-
-
Notifications
You must be signed in to change notification settings - Fork 0
Fix grounded follow-up audit findings #2
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
7092b95
e915a82
b0cacdc
652bccb
44fffc6
203d56b
2789aed
815ae7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| name: CI | ||
|
|
||
| "on": | ||
| pull_request: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ci-${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| check: | ||
| name: Check package | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
|
|
||
| - name: Set up Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: "1.3.14" | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: "24.x" | ||
| registry-url: "https://registry.npmjs.org" | ||
| package-manager-cache: false | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Check package | ||
| run: bun run check | ||
|
|
||
| - name: Check packed package | ||
| run: bun run pack:check |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import { AsyncLocalStorage } from "node:async_hooks"; | ||
|
|
||
| import { OutputCoordinator } from "./output-coordinator.js"; | ||
| import { chooseRenderer } from "./renderer.js"; | ||
| import { | ||
|
|
@@ -81,6 +83,9 @@ class LaquRuntime implements ProgressRuntime { | |
| #closed = false; | ||
| #processLifecycle: ProcessLifecycleLease | undefined; | ||
| readonly #handles = new Set<StoreTaskHandle>(); | ||
| readonly #taskCloseContext = new AsyncLocalStorage<StoreTaskHandle>(); | ||
| #activeScopedTasks = 0; | ||
| #closeRequestedByScopedTask = false; | ||
|
|
||
| constructor( | ||
| private readonly store: TaskStore, | ||
|
|
@@ -106,9 +111,10 @@ class LaquRuntime implements ProgressRuntime { | |
| throw new TypeError("task callback is required"); | ||
| } | ||
|
|
||
| const handle = this.createTask(title, options); | ||
| const handle = this.#createRootHandle(title, options); | ||
| this.#activeScopedTasks += 1; | ||
| try { | ||
| const result = await callback(handle); | ||
| const result = await this.#taskCloseContext.run(handle, () => callback(handle)); | ||
| if (this.#acceptsMutations()) { | ||
| handle.succeed(); | ||
| } | ||
|
|
@@ -125,17 +131,18 @@ class LaquRuntime implements ProgressRuntime { | |
| } | ||
| throw error; | ||
| } finally { | ||
| handle.dispose(); | ||
| this.#activeScopedTasks -= 1; | ||
| await this.flush(); | ||
| if (this.#shouldRunDeferredScopedClose()) { | ||
| this.#closeRequestedByScopedTask = false; | ||
| await this.close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| createTask(title: string, options: TaskOptions = {}): TaskHandle { | ||
| this.#assertAcceptsMutations(); | ||
| const id = this.store.createTask(title, options); | ||
| const handle = this.#createHandle(id); | ||
| handle.bindSignal(options.signal); | ||
| this.markDirty(true); | ||
| return handle; | ||
| return this.#createRootHandle(title, options); | ||
| } | ||
|
|
||
| log(message: string): void { | ||
|
|
@@ -152,6 +159,11 @@ class LaquRuntime implements ProgressRuntime { | |
| } | ||
|
|
||
| async close(): Promise<void> { | ||
| if (this.#shouldDeferScopedClose()) { | ||
| this.#closeRequestedByScopedTask = true; | ||
| await this.flush(); | ||
| return; | ||
| } | ||
| this.#closePromise ??= this.#closeOnce(); | ||
| await this.#closePromise; | ||
| } | ||
|
|
@@ -223,6 +235,24 @@ class LaquRuntime implements ProgressRuntime { | |
| }); | ||
| } | ||
|
|
||
| #shouldDeferScopedClose(): boolean { | ||
| return ( | ||
| this.#taskCloseContext.getStore() !== undefined && | ||
| this.#activeScopedTasks > 0 && | ||
| !this.#closing && | ||
| !this.#closed | ||
| ); | ||
| } | ||
|
Comment on lines
+238
to
+245
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. The check #shouldDeferScopedClose(): boolean {\n return (\n this.#taskCloseContext.getStore() !== undefined &&\n this.#activeScopedTasks > 0 &&\n !this.#closing &&\n !this.#closed\n );\n } |
||
|
|
||
| #shouldRunDeferredScopedClose(): boolean { | ||
| return ( | ||
| this.#closeRequestedByScopedTask && | ||
| this.#activeScopedTasks === 0 && | ||
| !this.#closing && | ||
| !this.#closed | ||
| ); | ||
| } | ||
|
|
||
| #acceptsMutations(): boolean { | ||
| return !this.#closing && !this.#closed; | ||
| } | ||
|
|
@@ -233,6 +263,15 @@ class LaquRuntime implements ProgressRuntime { | |
| } | ||
| } | ||
|
|
||
| #createRootHandle(title: string, options: TaskOptions): StoreTaskHandle { | ||
| this.#assertAcceptsMutations(); | ||
| const id = this.store.createTask(title, options); | ||
| const handle = this.#createHandle(id); | ||
| handle.bindSignal(options.signal); | ||
| this.markDirty(true); | ||
| return handle; | ||
| } | ||
|
|
||
| #createHandle(id: string): StoreTaskHandle { | ||
| let handle: StoreTaskHandle; | ||
| handle = new StoreTaskHandle( | ||
|
|
@@ -420,6 +459,16 @@ class StoreTaskHandle implements TaskHandle { | |
| this.onChange(true); | ||
| } | ||
|
|
||
| skip(message?: string): void { | ||
| this.assertWritable(); | ||
| this.store.update(this.id, { | ||
| status: "skipped", | ||
| ...(message === undefined ? {} : { message }), | ||
| }); | ||
| this.dispose(); | ||
| this.onChange(true); | ||
| } | ||
|
|
||
| child(title: string, options: TaskOptions = {}): TaskHandle { | ||
| this.assertWritable(); | ||
| return this.createChildHandle(this.id, title, options); | ||
|
|
||
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.
When a scoped task callback throws an error, the
catchblock handles updating the task status in the store, but the task handle itself is never disposed. This leads to a memory leak because the handle remains in the#handlesset indefinitely, and any registeredAbortSignallisteners are not cleaned up.\n\nTo fix this, we should explicitly callhandle.dispose()in thefinallyblock ofruntime.task. Sincedispose()is idempotent, this is safe to call even if the task succeeded and was already disposed.