Always flush the buffer on stop, and let unload finish (#577)

[GermanBluefox]

Fixes #577, found while answering #298. finish() is supposed to write everything still held in RAM when the adapter stops. Three things kept it from doing so — all only visible when maxLength > 0, since the default 0 buffers nothing.

1. The buffer was only flushed as a side effect

pushValuesIntoDB(id, list) appeared three times and all three sat inside the skipped branch or the writeNulls branch. There was no unconditional flush. A datapoint with neither lost its buffer.

Not an exotic case: writeNulls is forced off for every dialect with multiRequests === false — which is SQLite — so it hit every SQLite instance, plus anyone who unticked "Write NULL values on start/stop boundaries".

2. The counter was shared across datapoints

count lived in finish(), not in finishId(). Every datapoint incremented it and the flush ran on whichever callback brought it back to zero, using that callback's id. With 50 datapoints, one buffer was written and 49 were not — even with writeNulls on.

The same counter let allFinished() close the pool while later datapoints were still being dispatched. The dispatch is deliberately spread out (delay += dpcount % 50 === 0 ? 1000 : 0), which makes that more likely rather than less.

3. Unload could never complete

allFinished() was reachable only through that one flush callback. With nothing to write, the counter never moved, the unload callback never fired, and js-controller had to kill the instance. There was a guard for zero datapoints, none for "datapoints exist and none scheduled work".

The change

finishId() now counts its own pending writes, flushes its buffer unconditionally once they are done, and reports the datapoint complete. finish() counts datapoints rather than writes, and an allDispatched flag stops an early finisher from ending the run while the dispatch loop is still going.

Verification — and what it does not show

npm run check:ts, npm run lint (both passes) and prettier --check are clean; 192 unit tests pass. The full SQLite integration suite was run locally: 36 passing, 0 failing, with a clean Adapter normal terminated: true.

That run proves no regression, not the fix. SQLite forces writeNulls off, so Write 0 NULL appears 0 times in the log — but the suite's datapoints do produce skipped values, so the old code reached allFinished() through that branch and never exhibited the hang. The suite does not cover the "nothing to write" case.

Demonstrating the recovered data needs a dedicated test: a datapoint with maxLength > 0 and disableSkippedValueLogging on, a few values, a stop, then a query. Worth adding as a follow-up; I did not want to claim this run covers it.

Points 1 and 3 are plain readings of the control flow. Point 2 follows from the same code and would be worth confirming against an instance with many datapoints.

🤖 Generated with Claude Code

[GermanBluefox]

The MySQL job caught a regression in this PR, now fixed in cf90100.

uncaught exception: Cannot read properties of undefined (reading 'length')
    at SqlAdapter.pushValuesIntoDB (build/main.js:2181:19)
    at flushBuffer (build/main.js:1156:22)
    at Timeout.finishId [as _onTimeout] (build/main.js:1198:13)

Not every entry in sqlDPs is a configured datapoint. Four places create a stub with this.sqlDPs[id] ||= {} as SQLPointConfig — storeState, getHistory and the object view do it just to carry an index — and those have no list at all. pushValuesIntoDB() takes list.length on its first line.

The old flushes never reached them, because both sat behind skipped or config, which only a real datapoint has. Making the flush unconditional is exactly what exposed them. flushBuffer() now checks the buffer before using it and reports the datapoint done when there is nothing to write.

Worth recording why the local run missed it: the MySQL job runs five test files in sequence, so it stops the adapter five times with stubs present. The single-file SQLite suite I ran locally — 36 passing — stops it once, and the stubs were not there. A green single-dialect run was not the evidence I treated it as.

[GermanBluefox]

This PR is blocked on a release of @iobroker/aggregate, not on anything in it. Recording why, so the red MySQL job is not mistaken for a problem with the change.

The two failures are both getCounter returns the summed progression incl. a counter reset, in testMySQLExisting.js and testMySQLExistingNoNulls.js, answering 200.317… and 200.651… instead of 200.

That is this fix working. Once the adapter stops losing its boundary markers on shutdown, the NULL rows that writeNulls is supposed to write actually reach the database — and sendResponseCounter() had no concept of them:

where a NULL sits	before	correct
inside the window: 100 → null → 110	110	10
before the window	266.67	200

null coerces to 0 in that arithmetic and null > x is false, so the guard that drops a row before the window never fired for one. The fractional results here are the mild version; a restart inside a chart's window adds a whole meter reading to an energy counter.

Fixed in ioBroker/aggregate#5, merged to main as 2f53aaa. There is deliberately no release yet, so npm still serves the old 1.0.2 that ^1.0.2 resolves to, and CI keeps failing until one is cut and this adapter picks it up.

Nothing to change here in the meantime. Adjusting the test would only paper over a defect that is already fixed upstream.