fix: Close leaked file descriptors in SleepActivity and web server by brbla · Pull Request #869 · crosspoint-reader/crosspoint-reader

brbla · 2026-02-13T08:44:16Z

Summary

SleepActivity.cpp: Add missing file.close() calls in 3 code paths that open BMP files for sleep screen rendering but never close them before returning. Affects random custom sleep images, the /sleep.bmp fallback, and book cover sleep screens.
CrossPointWebServer.cpp: Add missing dir.close() in the delete handler when Storage.open() returns a valid FsFile that is not a directory.

Context

SdFat is configured with DESTRUCTOR_CLOSES_FILE=0, which means FsFile objects are not automatically closed when they go out of scope. Every opened file must be explicitly closed.

The SleepActivity leaks are particularly impactful because they occur on every sleep cycle. While ESP32 deep sleep clears RAM on wake, these leaks can still affect the current session if sleep screen rendering is triggered multiple times (e.g., cover preview, or if deep sleep fails to engage).

The web server leak in handleDelete() is a minor edge case (directory path that opens successfully but isDirectory() returns false), but it's still worth fixing for correctness.

Test plan

Verify sleep screen still renders correctly (custom BMP, fallback, cover modes)
Verify folder deletion still works via the web UI
Monitor free heap before/after sleep screen rendering to confirm no leak

🤖 Generated with Claude Code

SdFat is configured with DESTRUCTOR_CLOSES_FILE=0, so FsFile objects are not automatically closed when they go out of scope. Several code paths opened BMP files for sleep screen rendering but never closed them before returning: - renderCustomSleepScreen: random image and /sleep.bmp fallback - renderCoverSleepScreen: book cover BMP The web server delete handler also leaked an FsFile when the opened path was valid but not a directory. Co-Authored-By: Claude Opus 4.6 <[email protected]>

daveallie · 2026-02-16T12:17:17Z

Have you verified that folder deletion still works? I see it's unticked in your PR body?

brbla · 2026-02-16T12:28:57Z

I tested it again just to be sure. I can create and delete a folder on the card via the web UI without any problems. Only empty folders can be deleted, if there is a file or subfolder in it, the message appears: Failed to delete: Folder is not empty. Delete contents first.

The third point is related to my mistake, which is related to my attempt at localization into Czech, and therefore will not be relevant.

ngxson · 2026-02-16T16:51:50Z

I would be interested in doing researches about enabling DESTRUCTOR_CLOSES_FILE instead. Would appreciate if there are any insights on side effects if that's being enabled.

coderabbitai · 2026-02-22T07:09:02Z

No actionable comments were generated in the recent review. 🎉

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 786b438 and 4cdf45e.

📒 Files selected for processing (1)

src/activities/boot_sleep/SleepActivity.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: build

🔇 Additional comments (3)

src/activities/boot_sleep/SleepActivity.cpp (3)

83-90: Good cleanup on early return path.

Explicitly closing the file before returning prevents leaks in the random sleep image flow.

100-107: Cleanup looks solid for /sleep.bmp handling.

Ensures the file handle is released on both success and failure.

271-278: Proper file closure for cover BMP path.

Closes the file on both the success path and error path.

📝 Walkthrough

Walkthrough

Adds explicit file.close() calls to error handling and early-return paths in the BMP loading routine for custom sleep screens. These additions ensure file handles are properly released when bitmap header parsing fails or succeeds before returning from the function.

Changes

Cohort / File(s)	Summary
File Resource Management `src/activities/boot_sleep/SleepActivity.cpp`	Added `file.close()` calls in multiple error paths and early returns during BMP file loading to ensure proper resource cleanup when bitmap header parsing fails.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing leaked file descriptors in SleepActivity and web server components.
Description check	✅ Passed	The description is directly related to the changeset, providing detailed context about file descriptor leaks, affected code paths, and rationale for the fixes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

daveallie · 2026-02-22T07:14:47Z

I would be interested in doing researches about enabling DESTRUCTOR_CLOSES_FILE instead. Would appreciate if there are any insights on side effects if that's being enabled.

Looks to use about 5K of flash.

RAM:   [===       ]  31.5% (used 103100 bytes from 327680 bytes)
Flash: [=======   ]  68.9% (used 4513220 bytes from 6553600 bytes)

RAM:   [===       ]  31.5% (used 103100 bytes from 327680 bytes)
Flash: [=======   ]  68.9% (used 4518498 bytes from 6553600 bytes)

Functionally, we never actually intend to destruct a file without closing it, and indeed everywhere we open files we try to close them (this PR pointing out the few bad cases). I think we should move forward with enabling the flag, will open a new PR to do it in and merge this regardless.

…rosspoint-reader#869) - **SleepActivity.cpp**: Add missing `file.close()` calls in 3 code paths that open BMP files for sleep screen rendering but never close them before returning. Affects random custom sleep images, the `/sleep.bmp` fallback, and book cover sleep screens. - **CrossPointWebServer.cpp**: Add missing `dir.close()` in the delete handler when `Storage.open()` returns a valid `FsFile` that is not a directory. SdFat is configured with `DESTRUCTOR_CLOSES_FILE=0`, which means `FsFile` objects are **not** automatically closed when they go out of scope. Every opened file must be explicitly closed. The SleepActivity leaks are particularly impactful because they occur on every sleep cycle. While ESP32 deep sleep clears RAM on wake, these leaks can still affect the current session if sleep screen rendering is triggered multiple times (e.g., cover preview, or if deep sleep fails to engage). The web server leak in `handleDelete()` is a minor edge case (directory path that opens successfully but `isDirectory()` returns false), but it's still worth fixing for correctness. - [x] Verify sleep screen still renders correctly (custom BMP, fallback, cover modes) - [x] Verify folder deletion still works via the web UI - [ ] Monitor free heap before/after sleep screen rendering to confirm no leak 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Jan Bažant <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Dave Allie <[email protected]> (cherry picked from commit 3cb60aa)

…rosspoint-reader#869) ## Summary - **SleepActivity.cpp**: Add missing `file.close()` calls in 3 code paths that open BMP files for sleep screen rendering but never close them before returning. Affects random custom sleep images, the `/sleep.bmp` fallback, and book cover sleep screens. - **CrossPointWebServer.cpp**: Add missing `dir.close()` in the delete handler when `Storage.open()` returns a valid `FsFile` that is not a directory. ## Context SdFat is configured with `DESTRUCTOR_CLOSES_FILE=0`, which means `FsFile` objects are **not** automatically closed when they go out of scope. Every opened file must be explicitly closed. The SleepActivity leaks are particularly impactful because they occur on every sleep cycle. While ESP32 deep sleep clears RAM on wake, these leaks can still affect the current session if sleep screen rendering is triggered multiple times (e.g., cover preview, or if deep sleep fails to engage). The web server leak in `handleDelete()` is a minor edge case (directory path that opens successfully but `isDirectory()` returns false), but it's still worth fixing for correctness. ## Test plan - [x] Verify sleep screen still renders correctly (custom BMP, fallback, cover modes) - [x] Verify folder deletion still works via the web UI - [ ] Monitor free heap before/after sleep screen rendering to confirm no leak 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Jan Bažant <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Dave Allie <[email protected]>

## Summary * Enable `DESTRUCTOR_CLOSES_FILE` flag * We're never intending to not close files, so if we accidentally leave them open as they're destructured, this will help close them. ## Additional Context * As spotted in #869, there are cases where we were accidentally not closing files Looks to use about 5K of flash. ``` RAM: [=== ] 31.5% (used 103100 bytes from 327680 bytes) Flash: [======= ] 68.9% (used 4513220 bytes from 6553600 bytes) ``` ``` RAM: [=== ] 31.5% (used 103100 bytes from 327680 bytes) Flash: [======= ] 68.9% (used 4518498 bytes from 6553600 bytes) ``` --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? No

Merge branch 'master' into fork/brbla/fix/file-descriptor-leaks

4cdf45e

daveallie approved these changes Feb 22, 2026

View reviewed changes

daveallie merged commit 3cb60aa into crosspoint-reader:master Feb 22, 2026
6 checks passed

daveallie mentioned this pull request Feb 22, 2026

fix: Enable DESTRUCTOR_CLOSES_FILE flag #1075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Close leaked file descriptors in SleepActivity and web server#869

fix: Close leaked file descriptors in SleepActivity and web server#869
daveallie merged 2 commits intocrosspoint-reader:masterfrom
brbla:fix/file-descriptor-leaks

brbla commented Feb 13, 2026 •

edited

Loading

Uh oh!

daveallie commented Feb 16, 2026

Uh oh!

brbla commented Feb 16, 2026

Uh oh!

ngxson commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 22, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Uh oh!

daveallie commented Feb 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

brbla commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Test plan

Uh oh!

daveallie commented Feb 16, 2026

Uh oh!

brbla commented Feb 16, 2026

Uh oh!

ngxson commented Feb 16, 2026

Uh oh!

coderabbitai bot commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

daveallie commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brbla commented Feb 13, 2026 •

edited

Loading

coderabbitai bot commented Feb 22, 2026 •

edited

Loading

daveallie commented Feb 22, 2026 •

edited

Loading