Re: [PR] Send TypeScript SDK logs to stderr when the log socket disconnects [airflow]

["jason810496 (via GitHub)" <gi...@apache.org>]

jason810496 commented on code in PR #69302:
URL: https://github.com/apache/airflow/pull/69302#discussion_r3568286598


##########
ts-sdk/src/coordinator/log-channel.ts:
##########
@@ -47,31 +47,45 @@ export interface LogRecord {
 
 const DEFAULT_LOGGER_NAME = "ts-sdk";
 
+interface LogChannelState {
+  sock: Socket;
+  connected: boolean;
+  closed: boolean;
+}
+

Review Comment:
   I think we can keep the logging buffer part. Perhaps in the follow-up, we can keep the scope as-is for this PR to only focus on the redirecting the logs to stderr once receive any fatal error.
   
   More specific, the follow-up will target the logging that TS runtime produces before the socket connects as buffer then send to the log socket once both side connect.



##########
ts-sdk/src/coordinator/log-channel.ts:
##########
@@ -47,31 +47,45 @@ export interface LogRecord {
 
 const DEFAULT_LOGGER_NAME = "ts-sdk";
 
+interface LogChannelState {
+  sock: Socket;
+  connected: boolean;
+  closed: boolean;
+}
+
 export class LogChannel {
-  private readonly sock: Socket;
+  private readonly shared: LogChannelState;
   private readonly name: string;
   private readonly isRoot: boolean;
 
-  private constructor(sock: Socket, name: string, isRoot: boolean) {
-    this.sock = sock;
+  private constructor(shared: LogChannelState, name: string, isRoot: boolean) {
+    this.shared = shared;
     this.name = name;
     this.isRoot = isRoot;
-    if (isRoot) {
-      sock.on("error", (err) => {
-        process.stderr.write(`[${this.name}] log socket error: ${err.message}\n`);
-      });
-    }
   }
 
   static async connect(addr: string, name: string = DEFAULT_LOGGER_NAME): Promise<LogChannel> {
-    return new LogChannel(await connectTcp(addr), name, true);
+    const shared: LogChannelState = {
+      sock: await connectTcp(addr),
+      connected: true,
+      closed: false,
+    };
+    shared.sock.on("error", (err) => {
+      process.stderr.write(`[${name}] log socket error: ${err.message}\n`);
+    });

Review Comment:
   The `"error"` handler only writes to stderr; only the later `"close"` handler flips `shared.connected = false`. Node fires `"close"` a tick or so after `"error"` (empirically ~0.05ms later in a local repro), and in that gap a `send()` call still takes the "connected" branch and writes to an already-destroyed socket — the record is silently dropped instead of falling back to stderr as intended. Setting the flag in the `error` handler too closes the gap:
   
   ```suggestion
       shared.sock.on("error", (err) => {
         shared.connected = false;
         process.stderr.write(`[${name}] log socket error: ${err.message}\n`);
       });
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org