Skip to content

bug fixes: alerts and ingestion script - #1746

Merged
nikhilsinhaparseable merged 2 commits into
parseablehq:mainfrom
nikhilsinhaparseable:delete-and-load-alerts
Aug 9, 2026
Merged

bug fixes: alerts and ingestion script#1746
nikhilsinhaparseable merged 2 commits into
parseablehq:mainfrom
nikhilsinhaparseable:delete-and-load-alerts

Conversation

@nikhilsinhaparseable

@nikhilsinhaparseable nikhilsinhaparseable commented Aug 9, 2026

Copy link
Copy Markdown
Member
  1. load alerts at server startup
  2. delete alert for localfs
  3. ingestion script - start and stop path incorrect

Summary by CodeRabbit

  • Bug Fixes

    • Improved alert handling when storage tenant information is missing or uses the default value.
    • Alert and alert-state deletion now consistently uses the requested tenant and safely handles already-removed data.
    • Storage failures during deletion now provide clearer metastore errors.
    • Collector shutdown now verifies termination before removing its process record.
  • Improvements

    • Startup and status messages now provide direct commands for viewing logs, checking status, and stopping the collector across supported environments.
    • Configuration updates are safer and report failures more clearly.

1. load alerts at server startup
2. delete alert for localfs
3. ingestion script - start and stop path incorrect
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4046a779-f1ca-4b7d-a460-113ed9bde55a

📥 Commits

Reviewing files that changed from the base of the PR and between 91d01d9 and 93d6240.

📒 Files selected for processing (2)
  • scripts/ingest.ps1
  • scripts/ingest.sh

Walkthrough

The change normalizes alert tenant keys, makes alert deletion tenant-aware and idempotent, and strengthens collector lifecycle handling. The scripts now resolve paths, quote PowerShell paths, verify shutdown, preserve active PID files, and use safer configuration replacement.

Changes

Alert and collector corrections

Layer / File(s) Summary
Alert tenant normalization
src/alerts/mod.rs
Alert loading treats empty and default tenant keys as non-explicit and preserves named tenants. Tests cover these cases.
Tenant-aware alert deletion
src/metastore/metastores/object_store_metastore.rs, src/storage/mod.rs
Alert and alert-state deletion validates ULIDs, uses request-tenant paths, ignores missing entries, and propagates other storage errors. Tests cover tenant paths and repeated deletion.
PowerShell collector lifecycle
scripts/ingest.ps1
The script resolves paths, escapes single quotes, polls for shutdown, preserves active PID files, and replaces configuration through a unique backup file.
Shell collector lifecycle
scripts/ingest.sh
The script checks graceful and forced termination, waits for confirmed shutdown, preserves active PID files, and prints direct lifecycle commands.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: pratik50

Poem

A rabbit maps each tenant key,
And checks each path before shutdown.
PID files stay when processes run,
Safe backups guard the new config,
Alerts rest in their proper burrow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description lists the main objectives but omits the required Description details, rationale, testing status, comments, and documentation information. Use the repository template and add the change rationale, key implementation details, test results, and applicable comments or documentation updates.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the alert and ingestion script bug fixes covered by the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/ingest.ps1`:
- Line 176: Update the direct stop commands in scripts/ingest.ps1 at lines 176
and 319-321 and scripts/ingest.sh at line 260 to reuse the management-function
validation, including process identity checks, before signaling a PID. Ensure
PID files are removed only after a confirmed process stop; PowerShell must use
terminating error handling so Remove-Item cannot run after Stop-Process fails,
and the shell command must wait for process exit before deleting the PID file.
- Line 38: Escape the ingest script path before constructing SCRIPT_CMD by
moving the SCRIPT_PATH assignment below the ConvertTo-PowerShellSingleQuoted
helper definition and passing the joined path through that helper. Ensure paths
containing apostrophes remain valid PowerShell single-quoted strings.

In `@scripts/ingest.sh`:
- Line 259: Update the stop/status flow around the PID_FILE handling to wait for
the collector process to exit before removing the PID file, reusing
stop_collector’s existing wait and force-kill logic. Remove unnecessary shell
escaping for the fixed ./otelcol.pid path and ensure the displayed Stop command
does not delete the PID file immediately after kill.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e051c956-c402-44fd-bb0c-840992d33d98

📥 Commits

Reviewing files that changed from the base of the PR and between e32c0e9 and 91d01d9.

📒 Files selected for processing (5)
  • scripts/ingest.ps1
  • scripts/ingest.sh
  • src/alerts/mod.rs
  • src/metastore/metastores/object_store_metastore.rs
  • src/storage/mod.rs

Comment thread scripts/ingest.ps1
Comment thread scripts/ingest.ps1
Comment thread scripts/ingest.sh
@nikhilsinhaparseable
nikhilsinhaparseable merged commit 48ae5a3 into parseablehq:main Aug 9, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants