v0.2.12#58
Conversation
…g handler for dlt loggers reuse one handler per path (#56) * fix: added retry to pipeline.py to mitigate WinError 5 "access denied", shared RotatingFileHandler cache so root and dlt loggers reuse one handler per path, avoiding rotation "file in use" errors * improve error surfacing, platform-gate the permission error to Windows, testing --------- Co-authored-by: Stran Dutton <sdutton@specterops.io>
WalkthroughThis PR relocates the BloodHound Enterprise destination URL from secrets to config (updating ingest, examples, and docs), adds a shared rotating file-handler cache to CustomLogger to prevent duplicate handlers, and introduces retry logic with backoff for transient filesystem/permission errors in BasePipeline._run, with new tests for both. ChangesBHE destination URL moved to config
Logging handler caching and pipeline retry logic
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BasePipeline as BasePipeline._run
participant DltPipeline as dlt.pipeline.run
participant Logger
Caller->>BasePipeline: _run()
loop up to _MAX_TRANSIENT_RETRIES
BasePipeline->>DltPipeline: pipeline.run(...)
alt success
DltPipeline-->>BasePipeline: result
BasePipeline-->>Caller: return result
else transient OSError/PermissionError in cause chain
DltPipeline-->>BasePipeline: raise error
BasePipeline->>Logger: log warning, backoff
BasePipeline->>BasePipeline: sleep + retry
else non-transient error
DltPipeline-->>BasePipeline: raise error
BasePipeline-->>Caller: translate to ConfigException/ParseException/reraise
end
end
BasePipeline-->>Caller: reraise last error (retries exhausted)
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/openhound/core/logging.py (2)
325-368: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStale cached handlers on repeated
setup().If
setup()runs more than once with a differentlog_path/name(e.g. reconfiguration), the previously cached handler for the old path is removed from the loggers'handlerslists via.clear()but stays open inself._file_handlersforever — an accumulating, unclosed file descriptor per reconfiguration. Givensetup()is normally called once per process lifetime this is low-impact, but consider evicting/closing handlers for paths no longer referenced.🤖 Prompt for 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. In `@src/openhound/core/logging.py` around lines 325 - 368, The setup() method in logging.py is leaving stale file handlers open when it is called multiple times with a changed log_path or name. Update setup() to evict any no-longer-used handler from self._file_handlers and close it before replacing handlers on dlt_logger and root_logger, so repeated reconfiguration does not accumulate open file descriptors. Use the setup(), self._file_handlers, self.handlers, and log_file_path symbols to locate the cleanup logic.
398-405: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCache lookup/insert isn't atomic.
Two concurrent calls to
_get_file_handlerfor a new path could both miss the cache and each build aRotatingFileHandlerfor the same file before either is stored; the last write wins in the dict and the other handler is silently orphaned (open, unclosed, never used) — the exact multi-handler-on-one-file scenario this cache is meant to prevent. Low risk in current single-threaded setup flow, but worth athreading.Lockaround the check-and-set for defensive correctness.🔒 Optional lock-based fix
+import threading + class CustomLogger: def __init__(self, ...): ... self._file_handlers: dict[Path, "RotatingFileHandler"] = {} + self._file_handlers_lock = threading.Lock() def _get_file_handler(self, file_path: Path) -> "RotatingFileHandler": key = Path(file_path).resolve() - handler = self._file_handlers.get(key) - if handler is None: - handler = self._build_file_handler(file_path) - self._file_handlers[key] = handler - return handler + with self._file_handlers_lock: + handler = self._file_handlers.get(key) + if handler is None: + handler = self._build_file_handler(file_path) + self._file_handlers[key] = handler + return handler🤖 Prompt for 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. In `@src/openhound/core/logging.py` around lines 398 - 405, The shared handler cache in _get_file_handler is not atomic, so concurrent calls can create multiple RotatingFileHandler instances for the same path. Add a threading.Lock around the cache lookup/build/store sequence in Logging/_get_file_handler so the check-and-set is performed once per resolved Path key, and keep the existing _build_file_handler and _file_handlers behavior unchanged.src/openhound/core/pipeline.py (1)
44-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider dlt's built-in
retry_loadhelper for transient load-step retries.dlt already ships
dlt.pipeline.helpers.retry_load, designed to be used withtenacity.Retryingfor retryingrun/loadon transient errors. Reimplementing bespoke retry/backoff logic here duplicates functionality dlt already provides and misses tenacity's more battle-tested jitter/backoff strategies. Worth evaluating whetherretry_loadplus a customretry_if_exceptionpredicate (checking_transient_filesystem_cause) could replace this hand-rolled loop.🤖 Prompt for 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. In `@src/openhound/core/pipeline.py` around lines 44 - 91, The transient retry logic in _run is hand-rolling backoff and retry handling that dlt already provides. Replace the custom for-loop, warning, and sleep logic in Pipeline._run with dlt.pipeline.helpers.retry_load wired through tenacity.Retrying, and keep the existing transient predicate by using a custom retry_if_exception check based on _transient_filesystem_cause. Preserve the current special handling for PipelineStepFailed and PermissionError in _run, but let retry_load manage retry/backoff behavior instead of the bespoke _MAX_TRANSIENT_RETRIES path.
🤖 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.
Nitpick comments:
In `@src/openhound/core/logging.py`:
- Around line 325-368: The setup() method in logging.py is leaving stale file
handlers open when it is called multiple times with a changed log_path or name.
Update setup() to evict any no-longer-used handler from self._file_handlers and
close it before replacing handlers on dlt_logger and root_logger, so repeated
reconfiguration does not accumulate open file descriptors. Use the setup(),
self._file_handlers, self.handlers, and log_file_path symbols to locate the
cleanup logic.
- Around line 398-405: The shared handler cache in _get_file_handler is not
atomic, so concurrent calls can create multiple RotatingFileHandler instances
for the same path. Add a threading.Lock around the cache lookup/build/store
sequence in Logging/_get_file_handler so the check-and-set is performed once per
resolved Path key, and keep the existing _build_file_handler and _file_handlers
behavior unchanged.
In `@src/openhound/core/pipeline.py`:
- Around line 44-91: The transient retry logic in _run is hand-rolling backoff
and retry handling that dlt already provides. Replace the custom for-loop,
warning, and sleep logic in Pipeline._run with dlt.pipeline.helpers.retry_load
wired through tenacity.Retrying, and keep the existing transient predicate by
using a custom retry_if_exception check based on _transient_filesystem_cause.
Preserve the current special handling for PipelineStepFailed and PermissionError
in _run, but let retry_load manage retry/backoff behavior instead of the bespoke
_MAX_TRANSIENT_RETRIES path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bc677f2-fa12-4f4e-8ff1-7629b28b3a52
📒 Files selected for processing (12)
deployments/helm/openhound/README.mddeployments/helm/values.example.yamlexample-configurations/bloodhound-enterprise/.dlt-example/config.tomlexample-configurations/bloodhound-enterprise/.dlt-example/secrets_github.tomlexample-configurations/bloodhound-enterprise/.dlt-example/secrets_jamf.tomlexample-configurations/bloodhound-enterprise/.dlt-example/secrets_okta.tomlexample-configurations/bloodhound-enterprise/docker-compose.ymlsrc/openhound/core/logging.pysrc/openhound/core/pipeline.pysrc/openhound/destinations/bloodhound_enterprise/destination.pytests/test_log_handlers.pytests/test_pipeline_retry.py
💤 Files with no reviewable changes (3)
- example-configurations/bloodhound-enterprise/.dlt-example/secrets_okta.toml
- example-configurations/bloodhound-enterprise/.dlt-example/secrets_github.toml
- example-configurations/bloodhound-enterprise/.dlt-example/secrets_jamf.toml
2 participants
1. PR #57 by @StranDutton: fix: move non-sensitive data out of secrets.toml - BED-8838
2. PR #56 by @ktstrader: fix: added retry to pipeline.py to mitigate Windows "access denied" error, added log handler for dlt loggers reuse one handler per path - BED-8837(fix: added retry to pipeline.py to mitigate "access denied", added log handler for dlt loggers reuse one handler per path)
Important
Documentation (bloodhound-docs) update should be released at the same time (in relation to #57 OH config updates)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation