Skip to content

Commit

Permalink
fix: call fsync before rename (#1134)
Browse files Browse the repository at this point in the history
* fix: fsync on the file descriptor used for writing

According to the POSIX spec, one needs to call fsync on the file
descriptor used for writing, see:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fsync.html

Moreover, fail if database.db.tmp exists, since this is an indication
something related to database.db writing failed in the past and we did
not notice.

* fix: Put comment back above fsync

* fix: When database.db.tmp already exists, save it

* Add a unit test

* Prettier does not like CRLF (and I agree)

* Make the test work, now that my setup works

* Make prettier happy

* Update controller.test.ts

---------

Co-authored-by: Koen Kanters <koenkanters94@gmail.com>
  • Loading branch information
LaurentvdBos and Koenkk authored Aug 3, 2024
1 parent ac863e1 commit 1c9190a
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
15 changes: 13 additions & 2 deletions src/controller/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,20 @@ class Database {

const tmpPath = this.path + '.tmp';

fs.writeFileSync(tmpPath, lines.slice(0, -1)); // remove last newline, no effect if empty string
try {
// If there already exsits a database.db.tmp, rename it to database.db.tmp.<now>
const dateTmpPath = tmpPath + '.' + new Date().toISOString().replaceAll(':', '-');
fs.renameSync(tmpPath, dateTmpPath);

// If we got this far, we succeeded! Warn the user about this
logger.warning(`Found '${tmpPath}' when writing database, indicating past write failure; renamed it to '${dateTmpPath}'`, NS);
} catch {
// Nothing to catch; if the renameSync fails, we ignore that exception
}

const fd = fs.openSync(tmpPath, 'w');
fs.writeFileSync(fd, lines.slice(0, -1)); // remove last newline, no effect if empty string
// Ensure file is on disk /~https://github.com/Koenkk/zigbee2mqtt/issues/11759
const fd = fs.openSync(tmpPath, 'r+');
fs.fsyncSync(fd);
fs.closeSync(fd);
fs.renameSync(tmpPath, this.path);
Expand Down
19 changes: 19 additions & 0 deletions test/controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2185,6 +2185,25 @@ describe('Controller', () => {
expect(databaseContents().includes('groupID')).toBeFalsy();
});

it('Existing database.tmp should not be overwritten', async () => {
const databaseTmpPath = options.databasePath + '.tmp';
fs.writeFileSync(databaseTmpPath, 'Hello, World!');

await controller.start();
await mockAdapterEvents['deviceJoined']({networkAddress: 129, ieeeAddr: '0x129'});
controller.createGroup(1);

// The old database.db.tmp should be gone
expect(fs.existsSync(databaseTmpPath)).toBeFalsy();

// There should still be a database.db.tmp.<something>
const dbtmp = fs.readdirSync(TEMP_PATH).filter((value) => value.startsWith('database.tmp'));
expect(dbtmp.length).toBe(1);

// The database.db.tmp.<something> should still have our "Hello, World!"
expect(fs.readFileSync(getTempFile(dbtmp[0])).toString().startsWith('Hello, World!')).toBeTruthy();
});

it('Should create backup of databse before clearing when datbaseBackupPath is provided', async () => {
const databaseBackupPath = getTempFile('database.backup');
if (fs.existsSync(databaseBackupPath)) fs.unlinkSync(databaseBackupPath);
Expand Down

0 comments on commit 1c9190a

Please sign in to comment.