Skip to content

fix: compose internal lifecycle hooks in attachWorkspaceStoreLifecycle#11

Merged
ryanjosebrosas merged 4 commits intoa7u-pr-finalfrom
copilot/sub-pr-10
Mar 26, 2026
Merged

fix: compose internal lifecycle hooks in attachWorkspaceStoreLifecycle#11
ryanjosebrosas merged 4 commits intoa7u-pr-finalfrom
copilot/sub-pr-10

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 26, 2026

attachWorkspaceStoreLifecycle was overwriting internal.initialize/internal.close with hooks derived solely from lifecycleStores, silently dropping any hooks already on internal (e.g., pg.Pool close from a default Postgres workspace store).

Changes

  • Composed initialize: calls internal.initialize?.() first (only when the active workspace store matches the internal store, to avoid triggering unused Postgres initialization with override stores), then initializes each lifecycle store in order
  • Composed close: closes lifecycle stores in reverse inside individual try/catch blocks, then always calls internal.close?.() in a finally block — ensuring internal teardown (e.g., connection pool cleanup) is never skipped even if a store close fails; multiple errors are aggregated via AggregateError
  • Hook presence check: either store-level or internal hooks are sufficient to produce a composed function; neither side is silently dropped
// Before — internal.close was overwritten (pool leaked)
const close = lifecycleStores.some((s) => s.close !== undefined)
  ? async () => { for (const s of [...lifecycleStores].reverse()) await s.close?.(); }
  : undefined;

// After — internal.close always runs after store teardown, errors are aggregated
const close =
  hasStoreClose || input.internal.close !== undefined
    ? async () => {
        const errors: unknown[] = [];
        try {
          for (const store of [...lifecycleStores].reverse()) {
            try { await store.close?.(); } catch (e) { errors.push(e); }
          }
        } finally {
          try { await input.internal.close?.(); } catch (e) { errors.push(e); }
          if (errors.length === 1) throw errors[0];
          if (errors.length > 1) throw new AggregateError(errors, "Multiple errors occurred while closing lifecycle stores and internal resources.");
        }
      }
    : undefined;

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] [WIP] Address feedback on hardening app-boundary workspace auth for a7u fix: compose internal lifecycle hooks in attachWorkspaceStoreLifecycle Mar 26, 2026
Copilot AI requested a review from ryanjosebrosas March 26, 2026 14:53
@ryanjosebrosas ryanjosebrosas marked this pull request as ready for review March 26, 2026 15:02
Copilot AI review requested due to automatic review settings March 26, 2026 15:02
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes attachWorkspaceStoreLifecycle so it composes (rather than overwrites) internal.initialize/internal.close, ensuring internal lifecycle hooks aren’t silently dropped when lifecycle-enabled workspace stores are present.

Changes:

  • Add hasStoreInitialize/hasStoreClose checks to decide when to synthesize lifecycle functions.
  • Compose initialize to call internal.initialize first, then initialize lifecycle stores in order.
  • Compose close to close lifecycle stores in reverse order, then call internal.close last.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ryanjosebrosas and others added 2 commits March 26, 2026 23:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@ryanjosebrosas
Copy link
Copy Markdown
Owner

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown
Author

Copilot AI commented Mar 26, 2026

@copilot apply changes based on the comments in this thread

Both suggestions from that review have been applied:

  • initialize (4693aae): gates internal.initialize?.() behind input.workspaceStore === input.internal.workspaceStore, so override stores don't trigger unused Postgres initialization.
  • close (72ca60a): wraps store closes in individual try/catch inside a finally that always calls internal.close?.(), aggregating errors via AggregateError when multiple closes fail.

@ryanjosebrosas ryanjosebrosas merged commit 2b1decb into a7u-pr-final Mar 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants