Skip to content

Detect suspicious positional args from gflags bool flag misuse#4009

Merged
Ignition merged 5 commits intomasterfrom
flag_correction
Apr 7, 2026
Merged

Detect suspicious positional args from gflags bool flag misuse#4009
Ignition merged 5 commits intomasterfrom
flag_correction

Conversation

@Ignition
Copy link
Copy Markdown
Contributor

@Ignition Ignition commented Apr 7, 2026

gflags boolean flags silently ignore space-separated values: --flag false sets the flag to true and orphans "false" as a positional argument. This adds a startup check that warns (or errors with --strict-flag-check) when unexpected positional arguments are found after flag parsing.

Default is strict (error) for dev/CI; release config overrides to warn-only via flags.yaml.

gflags boolean flags silently ignore space-separated values:
`--flag false` sets the flag to true and orphans "false" as a
positional argument. This adds a startup check that warns (or
errors with --strict-flag-check) when unexpected positional
arguments are found after flag parsing.

Default is strict (error) for dev/CI; release config overrides
to warn-only via flags.yaml.
@Ignition Ignition self-assigned this Apr 7, 2026
@Ignition Ignition added CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=core Run coverage build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push CI -build=release -test=benchmark Run release build and benchmark on push CI -build=release -test=query_modules Run release build and query modules tests on push CI -build=coverage -test=clang_tidy labels Apr 7, 2026
@Ignition
Copy link
Copy Markdown
Contributor Author

Ignition commented Apr 7, 2026

Tracking

  • [Link to Epic/Issue]

Standard development

CI Testing Labels

  • Select the appropriate CI test labels (CI -build=build-name -test=test-suite)

Documentation checklist

  • Add the documentation label
  • Add the bug / feature label
  • Add the milestone for which this feature is intended
    • If not known, set for a later milestone
  • Write a release note, including added/changed clauses
    • Added new warning for incorrect flag usage, along with --strict-flag-check to force memgraph error if command line flags were incorrect. #4009
  • Add boolean flag syntax warning and --strict-flag-check docs documentation#1582

@Ignition Ignition added this to the mg-v3.10.0 milestone Apr 7, 2026
gflags bool flags require '=' syntax: '--flag=false', not '--flag false'.
The latter silently sets the flag to true. These all happened to pass the
intended value (true) so the bug was invisible.
@Ignition Ignition added feature feature and removed CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=core Run coverage build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push CI -build=release -test=benchmark Run release build and benchmark on push CI -build=release -test=query_modules Run release build and query modules tests on push CI -build=coverage -test=clang_tidy labels Apr 7, 2026
@Ignition Ignition added the CI -build=release -test=e2e Run release build and e2e tests on push label Apr 7, 2026
Multi-line patterns like:
  "--data-recovery-on-startup",
  "false",
silently set the flag to true (the opposite of intent). Use '=' syntax:
  "--data-recovery-on-startup=false",

Affected: replication, high_availability, system_replication tests.
@Ignition Ignition requested a review from as51340 April 7, 2026 12:54
@Ignition Ignition added the Docs needed Docs needed label Apr 7, 2026
The previous boolean flag fix revealed that this test was
accidentally running with data-recovery-on-startup=true (due to
gflags treating the bare --flag as true and ignoring the
space-separated "false"). Without FC and without data recovery,
replicas have no mechanism to restore databases after restart.

Add a data_recovery parameter to create_memgraph_instances_with_role_recovery
so the wo_fc test can explicitly opt in to data recovery while other
callers retain the default (false).
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 7, 2026

@Ignition Ignition added this pull request to the merge queue Apr 7, 2026
Merged via the queue into master with commit ede7930 Apr 7, 2026
41 checks passed
@Ignition Ignition deleted the flag_correction branch April 7, 2026 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI -build=release -test=e2e Run release build and e2e tests on push Docs needed Docs needed feature feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants