Skip to content

Conversation

@alecholmes
Copy link
Contributor

@alecholmes alecholmes commented Sep 3, 2025

Overview

The fleet feature locally caches user config files fetched from the control plane. To support hot reloading and rollback when configs change, the implementation may cache multiple versions of the config at any given time. Prior to this PR, there are effectively separate implementations for POSIX and Windows, which causes a few problems:

  • Harder to fix some fleet hot reload bugs. This PR is a precursor to an upcoming PR fixing fleet config updates bugs.
  • Less confidence around code changes working across all platforms (especially with testing being harder on Windows in practice)
  • Harder to actually reason about how the feature should work given the notably different approaches.

This PR refactors how the implementation tracks the various versions of configs, unifying around a single approach that works for both POSIX and Windows. This replaces symlinks (on POSIX) and assumptions around file ordering (on Windows) with a single approach of approximating symlink behavior in a cross-platform way. This change was designed to safely roll forward and rollback from older fluent-bit binaries.

I have tested this on both Linux and Windows.

Eng design

It is useful to explicitly track which config files are the stable and which ones are new. Fleet does this today on POSIX systems using symlinks (new.conf is just fetched and maybe invalid, old.conf is the last stable config):

drwx------ 2 alec alec  40 Aug 15 11:31 1755306650
-rw-r--r-- 1 alec alec 208 Aug 15 11:31 1755306650.conf
drwx------ 2 alec alec  40 Aug 15 11:34 1755308054
-rw-r--r-- 1 alec alec 208 Aug 15 11:34 1755308054.conf
-rw-r--r-- 1 alec alec 404 Aug 15 11:34 header.conf
lrwxrwxrwx 1 alec alec 106 Aug 15 11:34 new.conf -> /tmp/calyptia-fleet/4803d408a0d43a9281ef4ae7720dfd7a23003204d98a4a4ffedca2f036db7206/hello/1755308
054.conf
lrwxrwxrwx 1 alec alec 106 Aug 15 11:31 old.conf -> /tmp/calyptia-fleet/4803d408a0d43a9281ef4ae7720dfd7a23003204d98a4a4ffedca2f036db7206/hello/1755306
650.conf

Windows does not have symlinks so it instead implicitly relies on the number of files and their lexical order, e.g. it assumes the last 3 files in the config directory are the new, unstable configs.

This new approach approximates symlinks without using them. Instead of symlinks, it uses files that contain the path of the relevant config:

drwx------ 2 alec alec  40 Aug 15 11:31 1755306650
-rw-r--r-- 1 alec alec 208 Aug 15 11:31 1755306650.conf
drwx------ 2 alec alec  40 Aug 15 11:34 1755308054
-rw-r--r-- 1 alec alec 208 Aug 15 11:34 1755308054.conf
-rw-r--r-- 1 alec alec 404 Aug 15 11:34 header.conf
lrwxrwxrwx 1 alec alec 106 Aug 15 11:34 new.ref          # Fake symlink similar to new.conf in the old impl
lrwxrwxrwx 1 alec alec 106 Aug 15 11:31 old.ref          # Fake symlink similar to old.conf in the old impl

In the example above, the contents of the new.ref would be /tmp/calyptia-fleet/4803d408a0d43a9281ef4ae7720dfd7a23003204d98a4a4ffedca2f036db7206/hello/1755308 054.conf.

Binary rollout and rollback

We don't want failures or incorrect behavior when a customer upgrades their fluent-bit binary to include this change, or downgrades to an older binary. Incorrect behavior either is failure to work at all, or incorrectly using an older config.

Upgrade path:

  • When symlinks are detected, clear out the cached config directory. The latest config will be fetched and cached.

Downgrade path:

  • The newer *.ref files are ignored (since the code doesn't know about them). The latest config will be fetched and cached, and symlinks recreated.

Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

Summary by CodeRabbit

  • New Features

    • Ref-file based fleet config management replacing symlink flows, with atomic ref updates and legacy cleanup run on init.
  • Bug Fixes

    • Safer threaded reloads and error handling, recursive directory deletion, improved Windows path handling, and correct 64-bit timestamp formatting.
  • Refactor

    • Simplified base-directory handling and unified ref-based config dereference and load/commit/rollback flows.
  • Chores

    • Public API preserved; legacy macro names retained for compatibility.

@coderabbitai
Copy link

coderabbitai bot commented Sep 3, 2025

Walkthrough

Replaces symlink-based fleet config handling with a ref-file model, adds ref/deref APIs and recursive deletion, refactors base-dir allocation, adds legacy cleanup, atomic ref writes, safer threaded reloads, standardized 64-bit timestamp formatting, and extended logging. Public APIs/signatures remain unchanged.

Changes

Cohort / File(s) Summary
Plugin implementation
plugins/in_calyptia_fleet/in_calyptia_fleet.c
Replace symlink/filename flows with a ref-file approach: add fleet_config_ref_filename, fleet_config_deref, fleet_config_set_ref, calyptia_config_delete_by_ref, delete_dir; refactor base directory creation to return an allocated path; dereference refs for reads; atomic .tmp write + rename_file for ref updates; spawn detached reload threads with copied path and safer cleanup; add cleanup_legacy_config_state; standardize 64-bit timestamp formatting; improve logging and resource checks.
Public header / macros
plugins/in_calyptia_fleet/in_calyptia_fleet.h
Rename filename macros to legacy-prefixed variants: legacy_new_fleet_config_filename, legacy_cur_fleet_config_filename, legacy_old_fleet_config_filename (retain same expansions); keep existing public signatures unchanged.

Sequence Diagram(s)

sequenceDiagram
    participant Init as in_calyptia_fleet_init
    participant FS as Filesystem
    participant Ref as RefManager
    participant Thread as ReloadThread

    Note over Init,FS: Initialization performs legacy cleanup and creates base dir
    Init->>FS: cleanup_legacy_config_state()
    FS-->>Init: legacy artifacts removed
    Init->>FS: generate_base_fleet_directory() → allocated_path

    Note over Ref,FS: Resolve refs instead of reading symlinks
    Ref->>FS: fleet_config_deref("new"/"cur"/"old")
    FS-->>Ref: resolved config path

    Note over Ref,FS: Atomic ref update (write tmp -> rename)
    Ref->>FS: write temp ref file
    Ref->>FS: rename_file(tmp -> ref)

    Note over Ref,Thread: Async reload with copied path, safer cleanup on failure
    Ref->>Thread: execute_reload(copy_of_resolved_path)
    Thread->>FS: read config from copied path
    alt success
      Thread-->>Ref: reload success
    else failure
      Thread->>Ref: resume collectors / cleanup
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • edsiper
  • koleini
  • fujimotos

"I'm a rabbit in the code burrow, hopping free,
I swapped old links for refs beneath the tree.
Temp files renamed, threads launched with care,
Legacy debris cleared from every lair.
Hop, deploy, and log — config safe as can be! 🐇"

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.21% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the PR’s primary change: unifying fleet configuration file caching across platforms for the in_calyptia_fleet plugin, which matches the ref-file cross-platform caching refactor and legacy cleanup described in the summary. It is concise, specific, and relevant for a teammate scanning history.
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alecholmes
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

617-675: Ownership bug: execute_reload frees cfgpath on error; callers also free → double-free

execute_reload should not free cfgpath; callers already do. Remove all flb_sds_destroy(cfgpath) in execute_reload.

@@ static int execute_reload(struct flb_in_calyptia_fleet_config *ctx, flb_sds_t cfgpath)
-    if (parse_config_name_timestamp(ctx, cfgpath, &ctx->config_timestamp) != FLB_TRUE) {
-        flb_sds_destroy(cfgpath);
-        return FLB_FALSE;
-    }
+    if (parse_config_name_timestamp(ctx, cfgpath, &ctx->config_timestamp) != FLB_TRUE) {
+        return FLB_FALSE;
+    }
@@
-    if (reload == NULL) {
-        flb_sds_destroy(cfgpath);
-        return FLB_FALSE;
-    }
+    if (reload == NULL) {
+        return FLB_FALSE;
+    }
@@
-    if (reload->cfg_path == NULL) {
-        flb_free(reload);
-        flb_sds_destroy(cfgpath);
-        return FLB_FALSE;
-    }
+    if (reload->cfg_path == NULL) {
+        flb_free(reload);
+        return FLB_FALSE;
+    }
@@
-        flb_sds_destroy(cfgpath);
-        return FLB_FALSE;
+        return FLB_FALSE;

Note: Also remove the earlier free at Line 625 (same reason).

Also applies to: 1990-2000

🧹 Nitpick comments (5)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (5)

58-59: Guard S_ISDIR macro on Windows

Avoid redefinition warnings in environments that already provide S_ISDIR.

-#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR)
+#ifndef S_ISDIR
+#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR)
+#endif

210-233: Make new fleet_config_ helpers internal or declare them*

These helpers have external linkage but no header prototypes. Either make them static (preferred) or declare them in the header to avoid accidental external use.

-flb_sds_t fleet_config_ref_filename(struct flb_in_calyptia_fleet_config *ctx, char *ref_name)
+static flb_sds_t fleet_config_ref_filename(struct flb_in_calyptia_fleet_config *ctx, char *ref_name)
@@
-flb_sds_t fleet_config_deref(struct flb_in_calyptia_fleet_config *ctx, char *ref_name)
+static flb_sds_t fleet_config_deref(struct flb_in_calyptia_fleet_config *ctx, char *ref_name)
@@
-int fleet_config_set_ref(struct flb_in_calyptia_fleet_config *ctx,
+static int fleet_config_set_ref(struct flb_in_calyptia_fleet_config *ctx,

Also applies to: 239-270, 277-316


582-583: Minor: remove unused variable and favor safer copy

len is unused. Using snprintf prevents partial copy footguns.

-    ssize_t len;
@@
-    strncpy(realname, cfgpath, sizeof(realname)-1);
+    (void)snprintf(realname, sizeof(realname), "%s", cfgpath);

1343-1390: delete_dir: type consistency and robustness

  • Use size_t for idx to match entry_count.
  • Early-return true if directory is already gone after child deletions.
-    int idx;
+    size_t idx;
@@
-    if (rmdir(path) != 0) {
+    if (rmdir(path) != 0) {
         ret = FLB_FALSE;
     }

Optional: treat ENOENT from rmdir/unlink as success to make cleanup idempotent.


1514-1517: Commit flow: minor message and idempotency polish

  • Typo: “unable to delete old configuration by” → “by ref”.
  • Consider not erroring when old doesn’t exist; it’s harmless.
-        if (calyptia_config_delete_by_ref(ctx, "old") == FLB_FALSE) { 
-            flb_plg_error(ctx->ins, "unable to delete old configuration by");
+        if (calyptia_config_delete_by_ref(ctx, "old") == FLB_FALSE) {
+            flb_plg_error(ctx->ins, "unable to delete old configuration by ref");
             return FLB_FALSE;
         }

Also applies to: 1520-1524

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 35885ce and 92d74a7.

📒 Files selected for processing (2)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.c (16 hunks)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
  • GitHub Check: PR - fuzzing test
  • GitHub Check: pr-compile-centos-7
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
🔇 Additional comments (5)
plugins/in_calyptia_fleet/in_calyptia_fleet.h (1)

75-77: No internal usages of the old macros found
Verified via ripgrep: there are no occurrences of new_fleet_config_filename, cur_fleet_config_filename, or old_fleet_config_filename outside their legacy_* definitions. Safe to proceed with the renames.

plugins/in_calyptia_fleet/in_calyptia_fleet.c (4)

321-321: Time format cast fix LGTM

Switch to %lld with explicit cast avoids UB across platforms.


339-349: Ref-based detection and existence checks LGTM

Transition from filename macros to deref() reads clean and keeps semantics.

Also applies to: 366-376, 393-403, 476-490, 492-507, 509-523


1055-1056: Log formatting LGTM

Consistent %lld for time_t cast.


2325-2330: Legacy cleanup before dir creation: good, but confirm restart scenarios

Wiping the whole cache on legacy detection is fine. Please confirm docs call this out and tests cover “mixed-state” nodes (some timestamped dirs + legacy files).

Would you like a small test checklist to validate upgrade paths on Linux/Windows?

return NULL;
}

static int test_config_is_valid(struct flb_in_calyptia_fleet_config *ctx,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not called in practice and not helpful. Removing altogether.


#endif

static int cfl_array_qsort_conf_files(const void *arg_a, const void *arg_b)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to order files any more.

@alecholmes
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

584-595: Avoid basename portability pitfalls

basename() is non-standard on Windows. Use PATH_SEPARATOR scanning (as you already do elsewhere) to extract the filename.

-    fname = basename(realname);
+    {
+        char *sep = strrchr(realname, PATH_SEPARATOR[0]);
+        fname = sep ? sep + 1 : realname;
+    }
♻️ Duplicate comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

1416-1437: Make delete-by-ref idempotent: treat “no extension/matches” as success

Returning FLB_FALSE when the referenced path has no extension or when the glob finds nothing propagates failures into commit/rollback flows, even though there’s nothing to delete. This should be a no-op success.

-    if (ext == NULL) {
-        flb_plg_error(ctx->ins, "referenced config path has no extension: %s", config_path);
-        flb_sds_destroy(config_path);
-        return FLB_FALSE;
-    }
+    if (ext == NULL) {
+        flb_plg_debug(ctx->ins, "referenced config path has no extension: %s (nothing to delete)", config_path);
+        flb_sds_destroy(config_path);
+        return FLB_TRUE;
+    }
@@
-    if (confs == NULL) {
-        flb_plg_warn(ctx->ins, "config glob did not return any files: %s", config_path);
-        flb_sds_destroy(config_path);
-        return FLB_FALSE;
-    }
+    if (confs == NULL) {
+        flb_plg_debug(ctx->ins, "config glob returned no files: %s (nothing to delete)", config_path);
+        flb_sds_destroy(config_path);
+        return FLB_TRUE;
+    }
@@
-        if (unlink(ref_filename) != 0) {
+        if (unlink(ref_filename) != 0 && errno != ENOENT) {
             flb_plg_warn(ctx->ins, "unable to delete config ref file: %s", ref_filename);
         }

Also applies to: 1432-1437, 1456-1463

🧹 Nitpick comments (6)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (6)

297-316: Good fix on double-free/use-after-free; add newline/flush check and make write atomic

The ref write path looks solid now. Consider checking fputc/fflush and using write-then-rename for atomicity.

-    (void) fputc('\n', ref_file_ptr);
-    fclose(ref_file_ptr);
+    if (fputc('\n', ref_file_ptr) == EOF || fflush(ref_file_ptr) != 0) {
+        flb_plg_error(ctx->ins, "fleet_config_set_ref: failed to finalize reference file: %s", ref_filename);
+        fclose(ref_file_ptr);
+        flb_sds_destroy(ref_filename);
+        return FLB_FALSE;
+    }
+    fclose(ref_file_ptr);

Alternative (atomic): write to ".ref.tmp" then rename to ".ref".


484-493: Deduplicate exists_ with a tiny helper*

Three near-identical functions. Collapse to exists_ref(ctx, "new"/"cur"/"old") for maintainability.

-static int exists_new_fleet_config(struct flb_in_calyptia_fleet_config *ctx)
-{
-    int ret = FLB_FALSE;
-    flb_sds_t cfgnewname;
-    cfgnewname = fleet_config_deref(ctx, "new");
-    if (cfgnewname == NULL) {
-        return FLB_FALSE;
-    }
-    ret = access(cfgnewname, F_OK) == 0 ? FLB_TRUE : FLB_FALSE;
-    flb_sds_destroy(cfgnewname);
-    return ret;
-}
+static int exists_ref(struct flb_in_calyptia_fleet_config *ctx, const char *name)
+{
+    int ret = FLB_FALSE;
+    flb_sds_t p = fleet_config_deref(ctx, (char *)name);
+    if (p == NULL) { return FLB_FALSE; }
+    ret = access(p, F_OK) == 0 ? FLB_TRUE : FLB_FALSE;
+    flb_sds_destroy(p);
+    return ret;
+}

Then replace bodies with return exists_ref(ctx, "new"/"cur"/"old");

Also applies to: 501-510, 517-526


1345-1388: delete_dir: tighten loop types and error handling

  • Mixed signed/unsigned in the loop condition; compare size_t to size_t.
  • Optionally handle read-only files on Windows (remove FILE_ATTRIBUTE_READONLY before unlink).
-    for (idx = 0; idx < ((ssize_t)files->entry_count); idx++) {
+    for (idx = 0; idx < files->entry_count; idx++) {

1517-1549: Commit path robustness depends on idempotent delete_by_ref

With the above change, committing won’t fail when old artifacts are already gone. As a follow-up, consider treating unlink(new_ref_filename) ENOENT as success too.

-    unlink(new_ref_filename);
+    if (unlink(new_ref_filename) != 0 && errno != ENOENT) {
+        flb_plg_warn(ctx->ins, "unable to delete 'new' ref: %s", new_ref_filename);
+    }

1559-1594: Rollback path: mirror idempotency and ENOENT handling

Same reasoning as commit; rely on idempotent delete_by_ref and tolerate missing 'old' ref file.

-    unlink(old_ref_filename);
+    if (unlink(old_ref_filename) != 0 && errno != ENOENT) {
+        flb_plg_warn(ctx->ins, "unable to delete 'old' ref: %s", old_ref_filename);
+    }

1988-1998: load_fleet_config: small safety nit

execute_reload already duplicates cfg_path; good. Consider verifying the dereferenced path exists before calling execute_reload to avoid noisy logs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 92d74a7 and 0687187.

📒 Files selected for processing (2)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.c (16 hunks)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h
🔇 Additional comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)

342-352: Ref-based is_ checks look correct*

Using dereferenced paths instead of symlink semantics aligns with the new design.

Also applies to: 369-381, 396-408


2324-2329: Legacy cleanup ordering looks good

Running cleanup before directory creation matches the upgrade plan and prevents stale state from lingering.


1099-1115: Use abbreviated month (%b) and UTC conversion with timegm

Replace in plugins/in_calyptia_fleet/in_calyptia_fleet.c lines 1101–1102:

-        flb_strptime(fbit_last_modified, "%a, %d %B %Y %H:%M:%S GMT", &tm_last_modified);
-        last_modified = mktime(&tm_last_modified.tm);
+        if (!flb_strptime(fbit_last_modified, "%a, %d %b %Y %H:%M:%S GMT", &tm_last_modified)) {
+            flb_plg_error(ctx->ins, "invalid Last-Modified header: %s", fbit_last_modified);
+            ret = -1;
+            goto client_error;
+        }
+        last_modified = timegm(&tm_last_modified.tm);

Ensure flb_compat.h (or another project header) is included so that timegm is defined (mapped to _mkgmtime on Windows).

@alecholmes alecholmes requested a review from edsiper September 9, 2025 21:13

ref_file_ptr = fopen(ref_filename, "r");
if (ref_file_ptr == NULL) {
flb_plg_debug(ctx->ins, "fleet_config_deref: unable to open ref file: %s", ref_filename);
Copy link
Member

Choose a reason for hiding this comment

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

here I recommend moving flb_errno() as soon as fopen() fails (just to make sure no other call override errno value), putting this as an flb_plg_error() will give the user visibilty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)

2532-2532: Potential null-deref: avoid (void) *config

This dereferences config only to cast to void; if NULL, it can crash. Cast the pointer itself.

-    (void) *config;
+    (void) config;

1203-1216: HTTP Last-Modified parsing: use RFC7231 format and handle parse failure

The format should use abbreviated month ("%b"). Also check flb_strptime’s return.

-        flb_strptime(fbit_last_modified, "%a, %d %B %Y %H:%M:%S GMT", &tm_last_modified);
+        if (flb_strptime(fbit_last_modified, "%a, %d %b %Y %H:%M:%S GMT", &tm_last_modified) == NULL) {
+            flb_plg_warn(ctx->ins, "failed to parse Last-Modified header");
+            ret = -1;
+            goto client_error;
+        }

2399-2407: Free config_dir SDS in destroy
Under _WIN32 you call flb_sds_create_size()/flb_sds_printf on ctx->config_dir (plugins/in_calyptia_fleet/in_calyptia_fleet.c:2399-2407) but never free it in in_calyptia_fleet_destroy(). Add a flag to struct flb_in_calyptia_fleet_config (e.g. int config_dir_is_sds;), set it to FLB_TRUE immediately after flb_sds_printf, and in in_calyptia_fleet_destroy() do:

if (ctx->config_dir_is_sds) {
    flb_sds_destroy(ctx->config_dir);
}
♻️ Duplicate comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

660-662: basename() breaks MSVC builds; use a portable basename

basename() is unavailable on MSVC. Use a separator search that handles both '/' and '\'.

-    fname = basename(realname);
+    /* portable basename: handle both '/' and '\\' */
+    char *p1 = strrchr(realname, '/');
+    char *p2 = strrchr(realname, '\\');
+    fname = (p1 > p2 ? p1 : p2);
+    fname = fname ? fname + 1 : realname;
🧹 Nitpick comments (4)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (4)

394-405: Harden ref write: check fputc and flush before rename

Ensure the newline is written and data hit disk before rename; improves crash-safety.

-    fputc('\n', temp_file_ptr);
-    fclose(temp_file_ptr);
+    if (fputc('\n', temp_file_ptr) == EOF) {
+        flb_plg_error(ctx->ins, "fleet_config_set_ref: unable to write newline: %s", temp_filename);
+        fclose(temp_file_ptr);
+        unlink(temp_filename);
+        flb_sds_destroy(ref_filename);
+        flb_sds_destroy(temp_filename);
+        return FLB_FALSE;
+    }
+    fflush(temp_file_ptr);
+#ifndef _WIN32
+    int fd = fileno(temp_file_ptr);
+    if (fd >= 0) { (void) fsync(fd); }
+#endif
+    fclose(temp_file_ptr);

1518-1540: Deletion by ref: treat missing extension as a no-op

If the referenced path has no extension, consider it already cleaned; avoid emitting errors.

-    if (ext == NULL) {
-        flb_plg_error(ctx->ins, "referenced config path has no extension: %s", config_path);
-        flb_sds_destroy(config_path);
-        return FLB_FALSE;
-    }
+    if (ext == NULL) {
+        flb_plg_debug(ctx->ins, "referenced config path has no extension: %s", config_path);
+        flb_sds_destroy(config_path);
+        return FLB_TRUE;
+    }

1014-1018: Check flb_sds_printf() result for the Config-Version header

Avoid passing a NULL header value on allocation failure.

-    flb_sds_printf(&config_version, "%lld", (long long)ctx->config_timestamp);
+    if (flb_sds_printf(&config_version, "%lld", (long long)ctx->config_timestamp) == NULL) {
+        flb_sds_destroy(config_version);
+        flb_http_client_destroy(client);
+        flb_upstream_conn_release(u_conn);
+        return NULL;
+    }

273-304: Optional: avoid PATH_MAX truncation by reading the entire ref file

Using a fixed-size buffer can still truncate very long paths. Consider stat/fread into a dynamically sized buffer, then trim CRLF.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05cc743 and a391f55.

📒 Files selected for processing (2)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.c (31 hunks)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (5)
src/flb_sds.c (5)
  • flb_sds_create_size (92-95)
  • flb_sds_destroy (389-399)
  • flb_sds_printf (336-387)
  • flb_sds_create (78-90)
  • flb_sds_cat_safe (204-214)
src/flb_lib.c (1)
  • flb_context_get (993-999)
include/fluent-bit/flb_mem.h (2)
  • flb_calloc (84-96)
  • flb_free (126-128)
src/flb_input.c (1)
  • flb_input_collector_resume (2081-2137)
lib/cfl/src/cfl_array.c (1)
  • cfl_array_destroy (51-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
  • GitHub Check: PR - fuzzing test
  • GitHub Check: pr-compile-centos-7

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)

1416-1423: Infinite recursion on Windows globbing (use buf, not path)

Calling read_glob_win(path, ...) recurses with the same pattern forever.

Apply:

-        if (strchr(p1, '*')) {
-            if (read_glob_win(path, list) == NULL) {
+        if (strchr(p1, '*')) {
+            if (read_glob_win(buf, list) == NULL) {
                 cfl_array_destroy(list);
                 FindClose(hnd);
                 return NULL;
             }
             continue;
         }

1203-1206: HTTP Last-Modified parsing: wrong month token and timezone

RFC dates use abbreviated month (%b), and the value is GMT. mktime() treats it as local time.

Apply:

-        flb_strptime(fbit_last_modified, "%a, %d %B %Y %H:%M:%S GMT", &tm_last_modified);
-        last_modified = mktime(&tm_last_modified.tm);
+        flb_strptime(fbit_last_modified, "%a, %d %b %Y %H:%M:%S GMT", &tm_last_modified);
+#ifdef _WIN32
+        last_modified = _mkgmtime(&tm_last_modified.tm);
+#else
+        last_modified = timegm(&tm_last_modified.tm);
+#endif
+        if (last_modified == (time_t)-1) {
+            goto client_error;
+        }

2131-2155: Path traversal risk when writing fleet files

name comes from the control plane; it can escape fleetdir via “../”, absolute paths, or separators.

Apply a hard deny-list before composing the path:

 static int create_fleet_file(flb_sds_t fleetdir,
                              const char *name,
                              int nlen,
                              const char *b64_content,
                              int blen)
 {
     flb_sds_t fname;
     flb_sds_t dst;
     size_t dlen = 2 * blen;
     FILE *fp;
     int ret;
+    int i;
+
+    /* Basic path sanitization: reject absolute or traversal */
+    if (name == NULL || nlen <= 0) {
+        return -1;
+    }
+    /* absolute paths */
+    if (name[0] == '/' || name[0] == '\\') {
+        return -1;
+    }
+    /* traversal and separators */
+    for (i = 0; i < nlen; i++) {
+        if (name[i] == '/' || name[i] == '\\') {
+            return -1;
+        }
+    }
+    if (strstr(name, "..") != NULL) {
+        return -1;
+    }

Optional: normalize and verify the final path stays under fleetdir via realpath-equivalent before fopen.

♻️ Duplicate comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)

1519-1525: Deletion-by-ref: treat no-extension as no-op success (not error)

Aligns with “no matches” behavior and avoids noisy logs.

Apply:

-    if (ext == NULL) {
-        flb_plg_error(ctx->ins, "referenced config path has no extension: %s", config_path);
-        flb_sds_destroy(config_path);
-        return FLB_FALSE;
-    }
+    if (ext == NULL) {
+        flb_plg_debug(ctx->ins, "referenced config path has no extension: %s", config_path);
+        flb_sds_destroy(config_path);
+        return FLB_TRUE;
+    }

299-304: Trim CRLF, not just LF, when parsing ref files

On Windows, a stray '\r' can sneak into the path and break subsequent file ops.

Apply:

-        /* Remove trailing newline if present */
-        if (len > 0 && line[len - 1] == '\n') {
-            line[len - 1] = '\0';
-        }
+        /* Remove trailing newline(s), handle CRLF */
+        while (len > 0 && (line[len - 1] == '\n' || line[len - 1] == '\r')) {
+            line[--len] = '\0';
+        }

660-663: basename() is non-portable; avoid MSVC break

basename isn’t available on MSVC. Use a portable tail-extraction.

Apply:

-    snprintf(realname, sizeof(realname), "%s", cfgpath);
-    fname = basename(realname);
+    snprintf(realname, sizeof(realname), "%s", cfgpath);
+    fname = strrchr(realname, PATH_SEPARATOR[0]);
+    fname = fname ? fname + 1 : realname;
🧹 Nitpick comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

375-401: Open ref temp file in binary mode; ensure durability before rename

“w”/text can translate newlines on Windows; binary is safer. Optionally fsync dir on POSIX if durability matters.

Apply:

-    temp_file_ptr = fopen(temp_filename, "w");
+    temp_file_ptr = fopen(temp_filename, "wb");
@@
-    fputc('\n', temp_file_ptr);
+    fputc('\n', temp_file_ptr);
     fclose(temp_file_ptr);

If you need crash-safety across power loss, add fsync on the file and its parent dir before/after rename on POSIX.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a391f55 and 2e540e5.

📒 Files selected for processing (2)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.c (31 hunks)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (5)
src/flb_sds.c (5)
  • flb_sds_create_size (92-95)
  • flb_sds_destroy (389-399)
  • flb_sds_printf (336-387)
  • flb_sds_create (78-90)
  • flb_sds_cat_safe (204-214)
src/flb_lib.c (1)
  • flb_context_get (993-999)
include/fluent-bit/flb_mem.h (2)
  • flb_calloc (84-96)
  • flb_free (126-128)
src/flb_input.c (1)
  • flb_input_collector_resume (2081-2137)
lib/cfl/src/cfl_array.c (1)
  • cfl_array_destroy (51-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
  • GitHub Check: pr-compile-centos-7
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
  • GitHub Check: PR - fuzzing test
🔇 Additional comments (5)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (5)

155-202: Directory builder looks solid

Null checks and early frees are correct; SDS lifetime is handled properly.


316-330: Cross-platform replace-atomic rename looks good

Windows uses MoveFileExA with REPLACE_EXISTING|WRITE_THROUGH; POSIX uses rename(). Nice.


1108-1167: Timestamp-newness check: LGTM

Clear logic, correct cleanup on all paths, and ignores non-timestamped files.


1612-1697: Commit/rollback flows are coherent

Ref updates and cleanups are ordered correctly; errors are surfaced.


2441-2444: Legacy cleanup should be best-effort (already is)

Warn-and-continue is appropriate here.

Signed-off-by: Alec Holmes <alec@chronosphere.io>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)

1259-1261: Guard against uninitialized last_modified

When dst != NULL, last_modified isn’t set; writing it via time_last_modified stores garbage. Only assign when computed.

-    if (time_last_modified) {
-        *time_last_modified = last_modified;
-    }
+    if (time_last_modified && dst == NULL) {
+        *time_last_modified = last_modified;
+    }
♻️ Duplicate comments (2)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (2)

664-666: Windows build break: replace non-portable basename()

basename() isn’t available on MSVC. Use a portable separator search (handle both separators so cross-OS paths don’t choke).

-    fname = basename(realname);
+    /* Portable basename: prefer current-OS separator, but handle the other too */
+    fname = strrchr(realname, PATH_SEPARATOR[0]);
+#ifdef _WIN32
+    {
+        char *alt = strrchr(realname, '/');
+        if (alt && (!fname || alt > fname)) { fname = alt; }
+    }
+#else
+    {
+        char *alt = strrchr(realname, '\\');
+        if (alt && (!fname || alt > fname)) { fname = alt; }
+    }
+#endif
+    fname = fname ? fname + 1 : realname;

303-307: Fix Windows path parsing: strip CRLF when reading ref files

Ref files are written in text mode; on Windows they contain CRLF. Only trimming '\n' leaves a trailing '\r', breaking path lookups (access/stat/unlink). Trim both.

Apply:

-        /* Remove trailing newline if present */
-        if (len > 0 && line[len - 1] == '\n') {
-            line[len - 1] = '\0';
-        }
+        /* Remove trailing newline(s), handle CRLF */
+        while (len > 0 && (line[len - 1] == '\n' || line[len - 1] == '\r')) {
+            line[--len] = '\0';
+        }
🧹 Nitpick comments (4)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (4)

273-301: Avoid PATH_MAX truncation when dereferencing ref files

Using a fixed PATH_MAX buffer plus “newline required” check can reject valid long paths and older ref files lacking a trailing newline. Consider reading the whole file (fseek/ftell + fread) and accepting EOF-terminated lines.


398-399: Check newline write result when updating ref

Tiny robustness win: verify fputc succeeds before renaming.

-    fputc('\n', temp_file_ptr);
+    if (fputc('\n', temp_file_ptr) == EOF) {
+        flb_plg_error(ctx->ins, "fleet_config_set_ref: unable to write newline: %s", temp_filename);
+        fclose(temp_file_ptr);
+        unlink(temp_filename);
+        flb_sds_destroy(ref_filename);
+        flb_sds_destroy(temp_filename);
+        return FLB_FALSE;
+    }

1451-1496: Recursive delete: consider soft-failing on partial cleanup

Function returns FLB_FALSE on any single stat/unlink failure, but still best-effort deletes most entries. If callers treat FLB_FALSE as fatal, this could block upgrades due to a single stubborn file. Consider logging and continuing, and only fail if the root rmdir fails.


1647-1656: commit(): unlinking “new.ref” without checking errors is fine, but log errno on failure

Optional: add flb_errno() before warn to aid troubleshooting.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38b11b7 and 7883904.

📒 Files selected for processing (2)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.c (32 hunks)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/in_calyptia_fleet/in_calyptia_fleet.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (4)
src/flb_sds.c (5)
  • flb_sds_create_size (92-95)
  • flb_sds_destroy (389-399)
  • flb_sds_printf (336-387)
  • flb_sds_create (78-90)
  • flb_sds_cat_safe (204-214)
include/fluent-bit/flb_mem.h (2)
  • flb_calloc (84-96)
  • flb_free (126-128)
src/flb_input.c (1)
  • flb_input_collector_resume (2081-2137)
lib/cfl/src/cfl_array.c (1)
  • cfl_array_destroy (51-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
  • GitHub Check: PR - fuzzing test
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: pr-compile-centos-7
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
🔇 Additional comments (4)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (4)

320-334: Cross-platform atomic replace looks good

Switch to MoveFileExA with REPLACE_EXISTING/WRITE_THROUGH on Windows and rename() elsewhere is correct.


750-767: Threaded reload error handling improvements: LGTM

Good: checks for pthread_attr_init/setdetachstate/create, consistent cleanup, and collector resume on failure.

Also applies to: 770-777


1120-1171: Timestamp freshness check logic: LGTM (nice addition)

Clean globbing and comparison; returns true when directory is empty. Reads well.


2444-2449: Legacy cleanup on init: LGTM

Gracefully soft-failing and proceeding is the right call to avoid blocking startup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants