VxCentralScan: improve Importer correctness#8215
Merged
eventualbuddha merged 10 commits intomainfrom Apr 1, 2026
Merged
Conversation
`interpretSheet` either returns `Ok` or throws, so the `Result` is superfluous. The error check also threw anyway, to no effect. Removes the dead code.
There's no good reason to mutate the workspace state in `continueImport` if there is no scanning job in progress.
This method recurses, adding a stack frame for each iteration. This could theoretically be a problem if it waits a long time. Since there's no real benefit to the recursive version over a `while` loop, I'm switching to the latter.
If `finishBatch` were called again during one of the function's `await`s, it would not exit immediately since `currentBatch` has not yet been cleared, causing it to try to end the batch and remove the batch directory multiple times. Clearing `currentBatch` right away ensures that any re-entrant calls bail early.
Rather than just silently ignoring the error or a subsequent `finishBatch` error, log the errors as scan batch failures.
- It isn't re-entrant, so calling it again during its run would cause invalid state. - It wouldn't clean up the batch in the db or the scan target directory if anything failed before setting `this.currentBatch`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves correctness and edge-case handling in VxCentralScan’s backend Importer to better support future structural changes (e.g., interpreting while scanning) by tightening batch lifecycle management and error handling.
Changes:
- Simplifies
interpretSheetto return the direct interpretation result (instead of aResultwrapper that couldn’t actually represent failures). - Adds protection and cleanup for batch startup failures (
isStartingBatch, partial setup cleanup, and improved failure handling). - Improves scanning continuation robustness (explicit “no current batch” checks, better failure logging, non-recursive polling loop in test helper).
kshen0
approved these changes
Mar 31, 2026
Verifies that `finishBatch` clears `this.currentBatch` before async cleanup (endBatch, fsExtra.remove), preventing concurrent calls from doing duplicate work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Refs #8041
Before making structural changes to VxCentralScan to support interpreting while scanning, I wanted to improve the foundation a bit. This PR fixes a few issues Claude and I found while perusing
Importer. Mostly they're edge case handlng, but could represent real correctness issues.Demo Video or Screenshot
n/a
Testing Plan
Existing automated tests.