Skip to content

Commit 611d330

Browse files
committed
fix(Session): skip Reset() in destructor to prevent V8 JIT corruption
On Alpine/musl, calling Napi::ObjectReference::Reset() during GC finalization corrupts V8's JIT page allocations, causing SIGTRAP/SIGSEGV. The ObjectReference destructor is designed to be GC-safe, but explicitly calling Reset() during GC is not. This fix adds an in_destructor_ flag to Session so that Delete() skips the explicit Reset() call when called from the destructor, letting the ObjectReference destructor handle cleanup naturally instead. Also updates CI matrix: Node 23 -> 25 (current dev branch).
1 parent 16c999f commit 611d330

File tree

1 file changed

+24
-37
lines changed

1 file changed

+24
-37
lines changed

doc/todo/P02-investigate-flaky-native-crashes.md

Lines changed: 24 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ Affected tests: Various, shifting between runs due to Jest worker assignment
7777

7878
**Commit**: `e1a3fcb`
7979

80-
### Bug 2e: V8 JIT Corruption on Alpine During Jest Cleanup (FINAL FIX)
80+
### Bug 2e: V8 JIT Corruption on Alpine During Jest Cleanup
8181

8282
**The bug**: When Sessions are GC'd during Jest cleanup (process exit), their `Napi::ObjectReference` destructors call `Reset()`. On Alpine/musl, this corrupts V8's JIT page allocations.
8383

@@ -89,47 +89,33 @@ Affected tests: Various, shifting between runs due to Jest worker assignment
8989
4. V8 JIT corruption: `Check failed: it != jit_page_->allocations_.end()`
9090
5. **SIGSEGV** or **SIGABRT**
9191

92-
**Why it only happened on Alpine/musl**:
92+
**Partial Fix**: Hold strong references to Session JS objects during `DeleteAllSessions()` cleanup, preventing GC from destroying them while we iterate.
9393

94-
- Different GC timing during process exit
95-
- musl's allocator behavior differs from glibc
96-
- V8's JIT page management is more sensitive in this environment
94+
**Commit**: `413c93f`
9795

98-
**Key Insight**: The crash happens during Jest cleanup, NOT during normal test execution. Tests pass with `--forceExit` (which skips cleanup).
96+
### Bug 2f: Explicit Reset() in Destructor Path Corrupts V8 (FINAL FIX)
9997

100-
**The Fix**: Hold strong references to Session JS objects during `DeleteAllSessions()` cleanup, preventing GC from destroying them while we iterate. Then explicitly clear `database_ref_` for all sessions. When the temporary references are released, Sessions can be GC'd with already-empty `database_ref_`.
98+
**The bug**: Even with the `DeleteAllSessions()` fix, crashes still occurred when Sessions were GC'd without an explicit `db.close()` call. The root cause: `Session::Delete()` was calling `database_ref_.Reset()` during GC finalization, which corrupts V8's JIT on Alpine/musl.
10199

102-
```cpp
103-
void DatabaseSync::DeleteAllSessions() {
104-
std::set<Session *> sessions_copy;
105-
{
106-
std::lock_guard<std::mutex> lock(sessions_mutex_);
107-
sessions_copy = sessions_;
108-
sessions_.clear();
109-
}
100+
**Key Insight**: The `Napi::ObjectReference` destructor is designed to be GC-safe. But **explicitly** calling `Reset()` during GC finalization is NOT safe on Alpine/musl.
110101

111-
// Hold strong references to prevent GC during iteration
112-
std::vector<Napi::ObjectReference> session_refs;
113-
session_refs.reserve(sessions_copy.size());
114-
for (auto *session : sessions_copy) {
115-
session_refs.push_back(Napi::Persistent(session->Value()));
116-
}
102+
**The Fix**: Add an `in_destructor_` flag to Session. When `Delete()` is called from the destructor, skip the explicit `Reset()` call and let the ObjectReference destructor handle cleanup naturally.
117103

118-
// Pass 1: Clear SQLite sessions
119-
for (auto *session : sessions_copy) {
120-
if (session->GetSession()) {
121-
sqlite3session_delete(session->GetSession());
122-
session->session_ = nullptr;
123-
}
124-
}
104+
```cpp
105+
Session::~Session() {
106+
in_destructor_ = true; // Signal we're in destructor path
107+
Delete();
108+
}
109+
110+
void Session::Delete() {
111+
// ... existing cleanup code ...
125112

126-
// Pass 2: Clear database references (might trigger GC, but safe)
127-
for (auto *session : sessions_copy) {
128-
if (!session->database_ref_.IsEmpty()) {
129-
session->database_ref_.Reset();
130-
}
113+
// Only call Reset() if NOT in destructor path
114+
// On Alpine/musl, calling Reset() during GC corrupts V8's JIT
115+
if (!in_destructor_ && !database_ref_.IsEmpty()) {
116+
database_ref_.Reset();
131117
}
132-
// session_refs released here - Sessions can be GC'd with empty database_ref_
118+
// If in destructor, ObjectReference destructor handles cleanup (GC-safe)
133119
}
134120
```
135121

@@ -260,7 +246,8 @@ docker run --rm -v "$(pwd)":/host:ro node:24-alpine sh -c '
260246
| `fb283df` | Release `database_ref_` in DeleteAllSessions |
261247
| `dadbb86` | Release mutex before GC-triggering operations |
262248
| `e1a3fcb` | Remove Reset() from DeleteAllSessions (incomplete fix) |
263-
| (pending) | Hold refs during cleanup to prevent V8 JIT corruption |
249+
| `413c93f` | Hold refs during cleanup to prevent V8 JIT corruption |
250+
| (pending) | Skip Reset() in destructor path - let ObjectRef handle it |
264251

265252
---
266253

@@ -272,14 +259,14 @@ docker run --rm -v "$(pwd)":/host:ro node:24-alpine sh -c '
272259

273260
**Implementation**:
274261

275-
1. Commit the final fix (session_refs approach)
262+
1. Commit the final fix (in_destructor_ flag approach)
276263
2. Push to trigger CI
277264
3. Monitor Alpine test jobs
278265

279266
**Completion checklist**:
280267

281268
- [ ] Commit pushed
282-
- [ ] test-alpine jobs pass for all Node versions (20, 22, 23, 24)
269+
- [ ] test-alpine jobs pass for all Node versions (20, 22, 24, 25)
283270
- [ ] test-alpine jobs pass for both architectures (x64, arm64)
284271
- [ ] No SIGSEGV/SIGABRT crashes in 5+ consecutive runs
285272
- [ ] Move TPP to `doc/done/`

0 commit comments

Comments
 (0)