Skip to content

persistent logger - check fd before write in append() (dfs 2624) #9066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

persistent logger - check fd before write in append() (dfs 2624) #9066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/util/persistent_logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const P = require('./promise');
const semaphore = require('./semaphore');
const { NewlineReader } = require('./file_reader');
const dbg = require('./debug_module')(__filename);
const APPEND_ATTEMPTS_LIMIT = 5;

/**
* PersistentLogger is a logger that is used to record data onto disk separated by newlines.
Expand Down Expand Up @@ -105,10 +106,20 @@ class PersistentLogger {
* @param {string} data
*/
async append(data) {
const fh = await this.init();

const buf = Buffer.from(data + '\n', 'utf8');
await fh.write(this.fs_context, buf, buf.length);

for (let attempt = 0; attempt < APPEND_ATTEMPTS_LIMIT; ++attempt) {
const fh = await this.init();
//if another process has deleted the active file,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other process should not be able to delete the file if there is >= 1 process that exists as a writer. The reason is that the delete would happen only after _process would finish processing the active log file and in order to finish this the collect_and_process must have finished which must acquire an EXCLUSIVE lock before doing so.

//this process' _poll_active_file_change might have closed the fd
//in that case fd is -1
//in order to avoid inter-process locking, we just re-init
//the fd to the new active file.
if (fh.fd === -1) continue;
await fh.write(this.fs_context, buf, buf.length);
break;
}

this.local_size += buf.length;
}
Comment on lines +111 to 124
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Retry loop still loses data & can throw unhandled EBADF

  1. local_size is incremented even when no write was performed (e.g. all attempts hit a closed fd). This silently corrupts accounting and drops the log entry.
  2. Between the fd === -1 check and fh.write() another thread can still close the handle, causing an EBADF that will bubble up and abort the whole caller despite the retry logic.
  3. continue without a short delay turns the loop into busy-spin when the fd stays invalid.
  4. When all attempts fail, the method returns undefined as if everything succeeded – the caller has no way to know the line was lost.

A minimal, self-contained fix:

         const buf = Buffer.from(data + '\n', 'utf8');

-        for (let attempt = 0; attempt < APPEND_ATTEMPTS_LIMIT; ++attempt) {
-            const fh = await this.init();
-            //if another process has deleted the active file,
-            //this process' _poll_active_file_change might have closed the fd
-            //in that case fd is -1
-            //in order to avoid inter-process locking, we just re-init
-            //the fd to the new active file.
-            if (fh.fd === -1) continue;
-            await fh.write(this.fs_context, buf, buf.length);
-            break;
-        }
-
-        this.local_size += buf.length;
+        let written = false;
+        for (let attempt = 0; attempt < APPEND_ATTEMPTS_LIMIT; ++attempt) {
+            const fh = await this.init();
+
+            if (fh.fd === -1) {                 // handle already closed
+                await P.delay(1);               // avoid busy-loop
+                continue;
+            }
+
+            try {
+                await fh.write(this.fs_context, buf, buf.length);
+                written = true;
+                break;
+            } catch (err) {
+                if (err.code !== 'EBADF') throw err; // real IO error
+                // someone closed the fd after we checked – retry
+                await this.close();
+            }
+        }
+
+        if (!written) {
+            throw new Error(`append failed after ${APPEND_ATTEMPTS_LIMIT} attempts`);
+        }
+
+        this.local_size += buf.length;

This guarantees either:
• the line was durably appended and local_size reflects that, or
• an explicit error is surfaced to the caller.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (let attempt = 0; attempt < APPEND_ATTEMPTS_LIMIT; ++attempt) {
const fh = await this.init();
//if another process has deleted the active file,
//this process' _poll_active_file_change might have closed the fd
//in that case fd is -1
//in order to avoid inter-process locking, we just re-init
//the fd to the new active file.
if (fh.fd === -1) continue;
await fh.write(this.fs_context, buf, buf.length);
break;
}
this.local_size += buf.length;
}
const buf = Buffer.from(data + '\n', 'utf8');
let written = false;
for (let attempt = 0; attempt < APPEND_ATTEMPTS_LIMIT; ++attempt) {
const fh = await this.init();
if (fh.fd === -1) { // handle already closed
await P.delay(1); // avoid busy-loop
continue;
}
try {
await fh.write(this.fs_context, buf, buf.length);
written = true;
break;
} catch (err) {
if (err.code !== 'EBADF') throw err; // real IO error
// someone closed the fd after we checked – retry
await this.close();
}
}
if (!written) {
throw new Error(`append failed after ${APPEND_ATTEMPTS_LIMIT} attempts`);
}
this.local_size += buf.length;
🤖 Prompt for AI Agents 
In src/util/persistent_logger.js around lines 111 to 124, the retry loop
increments local_size even if no write occurs, can throw unhandled EBADF errors
due to race conditions, busy-spins without delay on invalid fd, and silently
fails without notifying the caller. Fix this by incrementing local_size only
after a successful write, adding a short delay before retrying on invalid fd,
catching and handling EBADF errors to retry properly, and throwing an explicit
error if all attempts fail to ensure the caller knows the write did not succeed.


Expand Down