Skip to content

Fix event manager race conditions and test teardown after mocha-to-vitest migration#375

Merged
mariuz merged 12 commits intomasterfrom
copilot/review-failing-test-mocha-vitest
Feb 17, 2026
Merged

Fix event manager race conditions and test teardown after mocha-to-vitest migration#375
mariuz merged 12 commits intomasterfrom
copilot/review-failing-test-mocha-vitest

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 16, 2026

After switching from mocha to vitest, tests failed intermittently due to vitest's different isolation/parallelism defaults exposing pre-existing race conditions in the Firebird event system that mocha's sequential single-process execution masked.

Event manager race conditions (lib/wire/fbEventManager.js)

The event loop's eventcallback can fire between any two operations on the main connection, pushing a spurious queEvents callback onto the queue. This misaligns the callback queue so subsequent operations consume the wrong response and hang.

  • close(): Null out eventcallback before closeEvents to prevent re-queuing from stale op_event notifications
  • _changeEvent(): Same fix — suppress eventcallback during closeEvents + queEvents, restore after completion
  • close() callback: Was never invoked on success, causing await-style callers to hang forever
// Before: stale op_event fires loop() between closeEvents and queEvents
_changeEvent(callback) {
    self.db.connection.closeEvents(this.eventid, function (err) {
        self.db.connection.queEvents(self.events, self.eventid, callback);
    })
}

// After: suppress event loop during the critical section
_changeEvent(callback) {
    var savedCallback = self.eventconnection.eventcallback;
    self.eventconnection.eventcallback = null;
    self.db.connection.closeEvents(this.eventid, function (err) {
        self.db.connection.queEvents(self.events, self.eventid, function (err, ret) {
            self.eventconnection.eventcallback = savedCallback;
            callback(err, ret);
        });
    })
}

Test fixes

  • test/index.js: Close event managers after each test; resilient afterAll with timeout + force socket close; fix Object.assign(config)Object.assign({}, config) no-op
  • test/service.js: Drain readable streams returned by service property setters before calling getStats — undrained streams hold Firebird server locks, causing "Lock time-out" on setReadonlyMode/setReadwriteMode

Vitest configuration (vitest.config.js)

  • isolate: false, fileParallelism: false, maxWorkers: 1 — match mocha's single-process sequential execution model
  • hookTimeout: 30000 — database creation hooks need headroom on CI
  • Ordered include list: service.js before index.js since service tests need exclusive DB access

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Increase beforeEach delay from 100ms to 1000ms and set describe-level
timeout to 15000ms for the Server properties block. After Shutdown and
BringOnline operations, the Firebird service needs more time to settle
before subsequent service operations can succeed.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title [WIP] Review failing test after mocha to vitest switch Fix intermittent timeout in service.js Server properties tests Feb 16, 2026
Copilot AI requested a review from mariuz February 16, 2026 23:49
Vitest runs test files in parallel by default, but mocha (the previous
test runner) ran them sequentially. test/index.js and test/service.js
share a Firebird server and interfere with each other when running
concurrently - service.js shuts down and modifies the database while
index.js tries to query it, causing random timeouts in both files.

Also reverts the unnecessary beforeEach delay increase and describe
timeout override from the previous attempt, since sequential execution
eliminates the root cause.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix intermittent timeout in service.js Server properties tests Fix vitest parallel file execution causing intermittent CI timeouts Feb 16, 2026
Copilot AI and others added 2 commits February 17, 2026 00:15
Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Root cause: vitest runs each test file in a separate worker process
(fork), causing different database names per worker and abrupt TCP
connection termination between files. On Node 22, this leaves Firebird
in a transient state where the service.js worker can't connect within
the 10s hook timeout.

Fix:
- vitest.config.js: add maxWorkers: 1 and isolate: false (vitest 4 API)
  to run all files in one process, matching mocha's original behavior
- test/index.js: fix Object.assign(config) -> Object.assign({}, config)
  to prevent config mutation now that modules are shared across files

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest parallel file execution causing intermittent CI timeouts Fix vitest test isolation: run all files in single process to match mocha behavior Feb 17, 2026
Copilot AI changed the title Fix vitest test isolation: run all files in single process to match mocha behavior Fix test isolation: prevent "Lock time-out" errors on read-only/read-write property tests Feb 17, 2026
With isolate: false, all test files shared the same module cache,
meaning test/index.js and test/service.js used the same database path.
Unclosed connections from test/index.js (especially event manager
connections) held locks that prevented service.js from changing the
database access mode (read-only/read-write).

Reverting to the default isolate: true gives each test file its own
module evaluation with a unique timestamp-based database path,
eliminating cross-file connection interference.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix test isolation: prevent "Lock time-out" errors on read-only/read-write property tests Fix vitest test isolation to prevent lock timeout errors in service tests Feb 17, 2026
The beforeAll hook in test/service.js calls Firebird.attachOrCreate()
to create the test database, which can take longer than the default
10000ms hookTimeout (inherited from testTimeout) on CI — especially
on Node 22 where each test file runs in its own forked process.

Setting hookTimeout: 30000 gives hooks 30 seconds for database
creation and connection setup while keeping testTimeout at 10 seconds
for individual tests.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest test isolation to prevent lock timeout errors in service tests Fix vitest config for mocha-to-vitest migration Feb 17, 2026
With isolate: true (vitest 4 default), the module cache is cleared
between test files but socket objects survive in the same worker.
test/index.js Events tests leak event manager connections that are
never closed. When modules are re-loaded, old sockets interfere with
new Firebird connections, causing attachOrCreate() to hang forever.

Using isolate: false avoids module cache clearing. Reordering to run
service.js BEFORE index.js ensures service tests (which need exclusive
database access for read-only/read-write mode changes) run first with
no competing connections. Index tests run last, so any leaked event
connections don't affect subsequent exclusive-access operations.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest config for mocha-to-vitest migration Fix vitest config: reorder test files and restore isolate: false Feb 17, 2026
The FbEventManager.close() method was missing a callback invocation on
success - it ended the socket but never called the callback, causing
fromCallback(cb => evtmgr.close(cb)) to hang forever.

Also added proper cleanup in all Events tests by calling evtmgr.close()
after each test. Previously, event managers created via db.attachEvent()
were abandoned without cleanup, leaking TCP sockets that could interfere
with subsequent test files sharing the same Firebird server.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest config: reorder test files and restore isolate: false Fix vitest config and event manager resource leaks after mocha-to-vitest migration Feb 17, 2026
When close() calls closeEvents on the main connection, the Firebird
server may also send a stale op_event notification on the event
connection before the socket is fully closed. The eventcallback handler
processes this and calls loop(false) -> queEvents, pushing a spurious
callback onto the main connection queue. The next operation (e.g.
auxConnection from a subsequent attachEvent) then gets the wrong
response from the queue, causing it to hang indefinitely.

Fix: Set eventconnection.eventcallback = null at the start of close(),
before calling closeEvents. The existing guard in the event connection
data handler (if self.eventcallback) safely handles the null case.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest config and event manager resource leaks after mocha-to-vitest migration Fix vitest test isolation and event manager resource leaks after mocha migration Feb 17, 2026
testProperty called service property setters (setReadonlyMode etc.)
which return readable streams via _createOutputStream, but never
drained them. Undrained streams keep the Firebird service operation
running, holding internal locks on the database. setReadonlyMode and
setReadwriteMode need exclusive access and fail when previous undrained
operations still hold the database.

Fix: drain the service output stream with readStreamAsync before
calling getStats.

Also make the Events afterAll resilient to connection queue corruption
from event operations by racing db.detach() against a 5s timeout and
force-closing the socket if detach didn't complete.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix vitest test isolation and event manager resource leaks after mocha migration Fix test failures after mocha-to-vitest migration Feb 17, 2026
Same race condition as close(): when _changeEvent sends closeEvents +
queEvents on the main connection, a stale op_event notification can
arrive on the event connection and trigger eventcallback -> loop(false)
-> push spurious queEvents onto the main connection queue. This
misaligns the callback queue so closeEvents's response gets consumed
by the wrong callback, and registerEvent's callback never fires.

Fix: Temporarily null out eventconnection.eventcallback at the start
of _changeEvent (matching the pattern in close()), and restore it
after queEvents completes so the event loop can resume normally.

Co-authored-by: mariuz <18359+mariuz@users.noreply.github.com>
Copilot AI changed the title Fix test failures after mocha-to-vitest migration Fix event manager race conditions and test teardown after mocha-to-vitest migration Feb 17, 2026
@mariuz mariuz marked this pull request as ready for review February 17, 2026 13:04
@mariuz mariuz merged commit 2620f2b into master Feb 17, 2026
1 of 5 checks 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.

2 participants