Copilot commented on code in PR #886: URL: https://github.com/apache/skywalking-banyandb/pull/886#discussion_r2591657389
########## fodc/internal/flightrecorder/flightrecorder.go: ########## @@ -0,0 +1,312 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the License. + +package flightrecorder + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "os" + "sync" + "unsafe" + + "github.com/apache/skywalking-banyandb/fodc/internal/poller" +) + +const ( + // DefaultBufferSize is the default number of snapshots to store + DefaultBufferSize = 1000 + // HeaderSize is the size of the file header in bytes + HeaderSize = 64 + // MagicNumber identifies the flight recorder file format + MagicNumber = uint32(0x464C5243) // "FLRC" + // Version is the file format version + Version = uint32(1) +) + +// Header represents the file header +type Header struct { + Magic uint32 // Magic number + Version uint32 // Format version + BufferSize uint32 // Number of slots in buffer + WriteIndex uint32 // Current write index (circular) + Count uint32 // Total number of entries written + _ [40]byte // Padding to 64 bytes +} + +// FlightRecorder implements a circular buffer using memory-mapped files +// to persist metrics data across crashes +type FlightRecorder struct { + file *os.File + header *Header + data []byte + bufferSize uint32 + slotSize uint32 + mu sync.RWMutex + path string +} + +// NewFlightRecorder creates a new FlightRecorder with the specified buffer size +func NewFlightRecorder(path string, bufferSize uint32) (*FlightRecorder, error) { + if bufferSize == 0 { + bufferSize = DefaultBufferSize + } + + // Calculate slot size: estimate max snapshot size (1MB per snapshot) + slotSize := uint32(1024 * 1024) // 1MB per slot + // Total file size: header + (bufferSize * slotSize) + fileSize := int64(HeaderSize) + int64(bufferSize)*int64(slotSize) + + // Open or create the file + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return nil, fmt.Errorf("failed to open flight recorder file: %w", err) + } + + // Get file info to check if it exists and has data + info, err := file.Stat() + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to stat file: %w", err) + } + + // If file is new or smaller than expected, resize it + if info.Size() < fileSize { + if err := file.Truncate(fileSize); err != nil { + file.Close() + return nil, fmt.Errorf("failed to resize file: %w", err) + } + } + + // Memory map the file + data, err := mmapFile(file, int(fileSize)) + if err != nil { + file.Close() Review Comment: File handle may be writable as a result of data flow from a [call to OpenFile](1) and closing it may result in data loss upon failure, which is not handled explicitly. ########## fodc/internal/flightrecorder/flightrecorder.go: ########## @@ -0,0 +1,312 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the License. + +package flightrecorder + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "os" + "sync" + "unsafe" + + "github.com/apache/skywalking-banyandb/fodc/internal/poller" +) + +const ( + // DefaultBufferSize is the default number of snapshots to store + DefaultBufferSize = 1000 + // HeaderSize is the size of the file header in bytes + HeaderSize = 64 + // MagicNumber identifies the flight recorder file format + MagicNumber = uint32(0x464C5243) // "FLRC" + // Version is the file format version + Version = uint32(1) +) + +// Header represents the file header +type Header struct { + Magic uint32 // Magic number + Version uint32 // Format version + BufferSize uint32 // Number of slots in buffer + WriteIndex uint32 // Current write index (circular) + Count uint32 // Total number of entries written + _ [40]byte // Padding to 64 bytes +} + +// FlightRecorder implements a circular buffer using memory-mapped files +// to persist metrics data across crashes +type FlightRecorder struct { + file *os.File + header *Header + data []byte + bufferSize uint32 + slotSize uint32 + mu sync.RWMutex + path string +} + +// NewFlightRecorder creates a new FlightRecorder with the specified buffer size +func NewFlightRecorder(path string, bufferSize uint32) (*FlightRecorder, error) { + if bufferSize == 0 { + bufferSize = DefaultBufferSize + } + + // Calculate slot size: estimate max snapshot size (1MB per snapshot) + slotSize := uint32(1024 * 1024) // 1MB per slot + // Total file size: header + (bufferSize * slotSize) + fileSize := int64(HeaderSize) + int64(bufferSize)*int64(slotSize) + + // Open or create the file + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return nil, fmt.Errorf("failed to open flight recorder file: %w", err) + } + + // Get file info to check if it exists and has data + info, err := file.Stat() + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to stat file: %w", err) + } + + // If file is new or smaller than expected, resize it + if info.Size() < fileSize { + if err := file.Truncate(fileSize); err != nil { + file.Close() Review Comment: File handle may be writable as a result of data flow from a [call to OpenFile](1) and closing it may result in data loss upon failure, which is not handled explicitly. ```suggestion closeErr := file.Close() if closeErr != nil { return nil, fmt.Errorf("failed to resize file: %v; additionally, failed to close file: %v", err, closeErr) } ``` ########## fodc/internal/flightrecorder/flightrecorder.go: ########## @@ -0,0 +1,312 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the License. + +package flightrecorder + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "os" + "sync" + "unsafe" + + "github.com/apache/skywalking-banyandb/fodc/internal/poller" +) + +const ( + // DefaultBufferSize is the default number of snapshots to store + DefaultBufferSize = 1000 + // HeaderSize is the size of the file header in bytes + HeaderSize = 64 + // MagicNumber identifies the flight recorder file format + MagicNumber = uint32(0x464C5243) // "FLRC" + // Version is the file format version + Version = uint32(1) +) + +// Header represents the file header +type Header struct { + Magic uint32 // Magic number + Version uint32 // Format version + BufferSize uint32 // Number of slots in buffer + WriteIndex uint32 // Current write index (circular) + Count uint32 // Total number of entries written + _ [40]byte // Padding to 64 bytes +} + +// FlightRecorder implements a circular buffer using memory-mapped files +// to persist metrics data across crashes +type FlightRecorder struct { + file *os.File + header *Header + data []byte + bufferSize uint32 + slotSize uint32 + mu sync.RWMutex + path string +} + +// NewFlightRecorder creates a new FlightRecorder with the specified buffer size +func NewFlightRecorder(path string, bufferSize uint32) (*FlightRecorder, error) { + if bufferSize == 0 { + bufferSize = DefaultBufferSize + } + + // Calculate slot size: estimate max snapshot size (1MB per snapshot) + slotSize := uint32(1024 * 1024) // 1MB per slot + // Total file size: header + (bufferSize * slotSize) + fileSize := int64(HeaderSize) + int64(bufferSize)*int64(slotSize) + + // Open or create the file + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return nil, fmt.Errorf("failed to open flight recorder file: %w", err) + } + + // Get file info to check if it exists and has data + info, err := file.Stat() + if err != nil { + file.Close() Review Comment: File handle may be writable as a result of data flow from a [call to OpenFile](1) and closing it may result in data loss upon failure, which is not handled explicitly. ########## fodc/internal/flightrecorder/flightrecorder.go: ########## @@ -0,0 +1,312 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the License. + +package flightrecorder + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "os" + "sync" + "unsafe" + + "github.com/apache/skywalking-banyandb/fodc/internal/poller" +) + +const ( + // DefaultBufferSize is the default number of snapshots to store + DefaultBufferSize = 1000 + // HeaderSize is the size of the file header in bytes + HeaderSize = 64 + // MagicNumber identifies the flight recorder file format + MagicNumber = uint32(0x464C5243) // "FLRC" + // Version is the file format version + Version = uint32(1) +) + +// Header represents the file header +type Header struct { + Magic uint32 // Magic number + Version uint32 // Format version + BufferSize uint32 // Number of slots in buffer + WriteIndex uint32 // Current write index (circular) + Count uint32 // Total number of entries written + _ [40]byte // Padding to 64 bytes +} + +// FlightRecorder implements a circular buffer using memory-mapped files +// to persist metrics data across crashes +type FlightRecorder struct { + file *os.File + header *Header + data []byte + bufferSize uint32 + slotSize uint32 + mu sync.RWMutex + path string +} + +// NewFlightRecorder creates a new FlightRecorder with the specified buffer size +func NewFlightRecorder(path string, bufferSize uint32) (*FlightRecorder, error) { + if bufferSize == 0 { + bufferSize = DefaultBufferSize + } + + // Calculate slot size: estimate max snapshot size (1MB per snapshot) + slotSize := uint32(1024 * 1024) // 1MB per slot + // Total file size: header + (bufferSize * slotSize) + fileSize := int64(HeaderSize) + int64(bufferSize)*int64(slotSize) + + // Open or create the file + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return nil, fmt.Errorf("failed to open flight recorder file: %w", err) + } + + // Get file info to check if it exists and has data + info, err := file.Stat() + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to stat file: %w", err) + } + + // If file is new or smaller than expected, resize it + if info.Size() < fileSize { + if err := file.Truncate(fileSize); err != nil { + file.Close() + return nil, fmt.Errorf("failed to resize file: %w", err) + } + } + + // Memory map the file + data, err := mmapFile(file, int(fileSize)) + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to memory map file: %w", err) + } + + // Read or initialize header + header := (*Header)(unsafe.Pointer(&data[0])) + + if header.Magic != MagicNumber { + // Initialize new header + header.Magic = MagicNumber + header.Version = Version + header.BufferSize = bufferSize + header.WriteIndex = 0 + header.Count = 0 + // Sync header immediately + if err := msync(data[:HeaderSize]); err != nil { + munmap(data) + file.Close() + return nil, fmt.Errorf("failed to sync header: %w", err) + } + } else if header.Version != Version { + munmap(data) + file.Close() Review Comment: File handle may be writable as a result of data flow from a [call to OpenFile](1) and closing it may result in data loss upon failure, which is not handled explicitly. ########## fodc/internal/flightrecorder/flightrecorder.go: ########## @@ -0,0 +1,312 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the License. + +package flightrecorder + +import ( + "encoding/binary" + "encoding/json" + "fmt" + "os" + "sync" + "unsafe" + + "github.com/apache/skywalking-banyandb/fodc/internal/poller" +) + +const ( + // DefaultBufferSize is the default number of snapshots to store + DefaultBufferSize = 1000 + // HeaderSize is the size of the file header in bytes + HeaderSize = 64 + // MagicNumber identifies the flight recorder file format + MagicNumber = uint32(0x464C5243) // "FLRC" + // Version is the file format version + Version = uint32(1) +) + +// Header represents the file header +type Header struct { + Magic uint32 // Magic number + Version uint32 // Format version + BufferSize uint32 // Number of slots in buffer + WriteIndex uint32 // Current write index (circular) + Count uint32 // Total number of entries written + _ [40]byte // Padding to 64 bytes +} + +// FlightRecorder implements a circular buffer using memory-mapped files +// to persist metrics data across crashes +type FlightRecorder struct { + file *os.File + header *Header + data []byte + bufferSize uint32 + slotSize uint32 + mu sync.RWMutex + path string +} + +// NewFlightRecorder creates a new FlightRecorder with the specified buffer size +func NewFlightRecorder(path string, bufferSize uint32) (*FlightRecorder, error) { + if bufferSize == 0 { + bufferSize = DefaultBufferSize + } + + // Calculate slot size: estimate max snapshot size (1MB per snapshot) + slotSize := uint32(1024 * 1024) // 1MB per slot + // Total file size: header + (bufferSize * slotSize) + fileSize := int64(HeaderSize) + int64(bufferSize)*int64(slotSize) + + // Open or create the file + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + return nil, fmt.Errorf("failed to open flight recorder file: %w", err) + } + + // Get file info to check if it exists and has data + info, err := file.Stat() + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to stat file: %w", err) + } + + // If file is new or smaller than expected, resize it + if info.Size() < fileSize { + if err := file.Truncate(fileSize); err != nil { + file.Close() + return nil, fmt.Errorf("failed to resize file: %w", err) + } + } + + // Memory map the file + data, err := mmapFile(file, int(fileSize)) + if err != nil { + file.Close() + return nil, fmt.Errorf("failed to memory map file: %w", err) + } + + // Read or initialize header + header := (*Header)(unsafe.Pointer(&data[0])) + + if header.Magic != MagicNumber { + // Initialize new header + header.Magic = MagicNumber + header.Version = Version + header.BufferSize = bufferSize + header.WriteIndex = 0 + header.Count = 0 + // Sync header immediately + if err := msync(data[:HeaderSize]); err != nil { + munmap(data) + file.Close() Review Comment: File handle may be writable as a result of data flow from a [call to OpenFile](1) and closing it may result in data loss upon failure, which is not handled explicitly. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
