Skip to content

feat(debug): add close() method to DebugSession for resource lifecycle#820

Open
luojiyin1987 wants to merge 2 commits into
OWASP:mainfrom
luojiyin1987:refactor/debug-session-close
Open

feat(debug): add close() method to DebugSession for resource lifecycle#820
luojiyin1987 wants to merge 2 commits into
OWASP:mainfrom
luojiyin1987:refactor/debug-session-close

Conversation

@luojiyin1987

@luojiyin1987 luojiyin1987 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

What

Add close(): void to the DebugSession type. Both enabled and disabled sessions return a no-op close().

Why

Establishes the lifecycle API needed before moving the enabled debug logger to a stream-backed implementation. In this PR, close() is intentionally a no-op on both paths; actual cleanup will be implemented in the follow-up stream refactor.

Changes

  • src/output/debug.ts: Add close to DebugSession type and both return paths

Testing

  • TypeScript compiles cleanly
  • No behavior change (all close() implementations are no-ops)
  • All existing tests unaffected

Closes #819

@luojiyin1987 luojiyin1987 requested a review from sonukapoor as a code owner July 8, 2026 02:11

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for tackling the lifecycle gap - the no-op on the disabled path is exactly right. A couple of things before this goes in though.

The enabled path: right now createDebugLogger uses fs.appendFileSync on every log call, so there's no open file handle to close. The close: () => {} on the enabled path is also a no-op today. That's technically correct, but the PR description says this is for "proper resource cleanup" - so I want to make sure the stream refactor (switching to fs.createWriteStream) is coming in a companion PR. If it is, this makes sense as a foundation. If appendFileSync is staying, I'd rather add close() in the PR that actually needs it so the API doesn't promise something it doesn't deliver. Could you clarify the plan?

Also worth adding a test for the new close() on both paths - even a quick assertion that calling it doesn't throw would document the expected no-op behavior.

@luojiyin1987

luojiyin1987 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. I added a small contract test covering close() on both enabled and disabled debug sessions, documenting the current no-op behavior.

To clarify the plan: this PR is only introducing the lifecycle contract. The actual resource cleanup will come in a follow-up PR that switches the enabled debug logger from fs.appendFileSync to a stream-backed implementation and wires close() to the stream cleanup path. I also updated the PR description to make that sequencing explicit.

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding the test - asserting that close() returns undefined on both paths is exactly the right way to document the no-op behavior. The implementation is technically correct today since appendFileSync has no open handle to close. If a future PR switches to fs.createWriteStream, that's the right time to wire up a real close. Happy to merge this as-is.

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.

feat(debug): add close() method to DebugSession for resource lifecycle

2 participants