Fix incorrect resume cfg write #2352
Conversation
- Remove `current` and `currentIndex` fields (commented out). - Add `completedIndex` (tracks number of results actually printed) and `lastPrinted` (stores the last host that was output). - Update resume logic in `Runner.RunEnumeration`: - Resume state is now updated **after** user‑visible output is written, preventing skipped targets on abrupt interruption. - Record `lastPrinted` and increment `completedIndex` when a response is printed. - Disable the old pre‑output resume tracking in `processItem` (commented out). - Adjust `SaveResumeConfig` to persist the new fields (`completedIndex` and `lastPrinted`) instead of the removed ones. - Add detailed comment explaining the rationale for updating resume state at the latest commit point.
WalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
participant Runner
participant Output
participant Persistence
rect rgba(211,244,255,0.6)
Note over Runner,Output: Resume advancement deferred until after printing
end
Runner->>Output: process input -> produce/print result
alt printed successfully
Note right of Output: After successful print, Runner updates resume state
Output-->>Runner: printed
Runner->>Persistence: save `lastPrinted` (persist to `resume.cfg`)
Persistence-->>Runner: ack
else print error or skip
Output-->>Runner: error/no advancement
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runner/runner.go (1)
1305-1332: Critical: Resume functionality is now disabled.Commenting out the resume-skipping logic (lines 1306-1310) means that resume will not skip already-processed items on restart. While the PR description mentions this is a "partial fix," disabling resume skipping entirely breaks the resume feature.
Missing components for resume to work:
- Initialization logic: When resume config is loaded,
completedIndexandlastPrintedneed to be initialized fromIndexandResumeFrom.- Skip logic: Code needs to skip processing items until reaching the resume point (based on
completedIndexor matchinglastPrinted).The new approach (lines 1099-1113) correctly records progress after output, but without skip logic, users will reprocess all items from the beginning on every resume, defeating the purpose.
🔎 Expected resume flow
When resume is loaded, initialization should occur:
if r.options.ShouldLoadResume() && r.options.resumeCfg.Index > 0 { r.options.resumeCfg.completedIndex = r.options.resumeCfg.Index r.options.resumeCfg.lastPrinted = r.options.resumeCfg.ResumeFrom gologger.Debug().Msgf("Resuming at position %d: %s\n", r.options.resumeCfg.Index, r.options.resumeCfg.ResumeFrom) }Then in
processItem, skip logic should check:if r.options.resumeCfg != nil { // Skip items until we reach the resume point if r.options.resumeCfg.completedIndex > 0 { r.options.resumeCfg.completedIndex-- return nil } }Or alternatively match against
lastPrintedto find the resume point.
🧹 Nitpick comments (1)
runner/resume.go (1)
6-7: Remove commented-out code.The commented fields
currentandcurrentIndexshould be deleted entirely rather than left as comments. Commented code clutters the codebase and can cause confusion.🔎 Proposed cleanup
type ResumeCfg struct { ResumeFrom string Index int - // current string - // currentIndex int completedIndex int // number of results actually printed lastPrinted string // last host printed }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignorerunner/resume.gorunner/runner.go
🔇 Additional comments (1)
runner/runner.go (1)
1099-1113: Good approach: updating resume state after output commit point.The rationale is sound—updating resume state after output is printed prevents skipped targets on abrupt interruption. The
resp.Inputfield reliably contains the original input identifier, as it is consistently set toorigInputthroughout the codebase, making it suitable for resume tracking.
runner/resume.go
Outdated
| // current string | ||
| // currentIndex int |
There was a problem hiding this comment.
this is breaking change, would be better if you can handle resume cases also
There was a problem hiding this comment.
Thanks for the review. When I searched the entire codebase to see where those two parameters were actually used for reading and parsing the resume related logic, I couldn’t find any occurrences at all. That left me genuinely confused about how the resume logic is even being parsed.
There was a problem hiding this comment.
this is breaking change, would be better if you can handle resume cases also
Please take a look at #2345 (comment). The issue can be reproduced using the current stable release of httpx, and the resume logic is already broken.
There was a problem hiding this comment.
@ayuxsec thanks for looking into the details. Here's what I meant anyone using resume config in SDK or some integration might break if we change the struct behavior. I know this is a partial fix for your issue, but we should ensure backward compatibility.
You can keep removed current since lastPrinted now handles that for saving.
Here's what the skip logic should look like:
processItem := func(k string) error {
if r.options.resumeCfg != nil && r.options.ShouldLoadResume() && r.options.resumeCfg.Index > 0 {
r.options.resumeCfg.currentIndex++
if r.options.resumeCfg.currentIndex <= r.options.resumeCfg.Index {
return nil // here we can skip already processed items
}
}
protocol := r.options.protocol
// ... rest of existing code
}
do give a try and let me know if it make sense.
runner/runner.go
Outdated
| // NOTE: Resume state is updated at the *latest commit point* (i.e. after | ||
| // user-visible output is produced), not during input ingestion or scheduling. | ||
| // | ||
| // Reason: | ||
| // - Updating resume earlier (when reading from streamChan / processItem) | ||
| // causes skipped targets on resume if execution is interrupted. | ||
| // - A missed index is acceptable (user may re-scan one host), | ||
| // but a skipped index is catastrophic (user silently misses hosts). | ||
| // | ||
| // Therefore, we only advance resume state after the result has been | ||
| // irrevocably committed to output. |
There was a problem hiding this comment.
comments is nicely explains the bug, but we can keep it short concise eg:
// Update resume state after output to avoid skipping unprocessed targets on interrupt
Co-authored-by: Nakul Bharti <knakul853@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @runner/resume.go:
- Around line 3-10: Resume currently saves ResumeCfg but never skips processed
items; update processItem to consult r.options.resumeCfg (ResumeFrom/Index)
before work and return early when item should be skipped. Locate the processItem
function and add a check using r.options.resumeCfg: if it's non-nil and either
Index > 0 (use a running counter to compare to Index) or ResumeFrom is set (call
a helper shouldSkip(currentKey, ResumeFrom) or implement equivalent string
comparison), then return nil to avoid reprocessing; ensure completedIndex and
lastPrinted continue to be updated when processing resumes past the skip point.
🧹 Nitpick comments (3)
runner/resume.go (3)
4-5: Clarify the relationship between exported and unexported fields.The struct now has two sets of fields that appear to track similar information:
Index/ResumeFrom(exported) andcompletedIndex/lastPrinted(unexported). From the context, it appears that the exported fields are used for serialization/deserialization while the unexported fields track runtime state. This dual-field design can be confusing.Consider adding documentation to clarify:
- Which fields are persisted to
resume.cfg- Which fields are used for internal state tracking
- How the values flow between these field pairs
📝 Example documentation
type ResumeCfg struct { + // ResumeFrom is the last successfully processed target (persisted to disk) ResumeFrom string + // Index is the count of successfully processed targets (persisted to disk) Index int // current string // currentIndex int + // completedIndex tracks the number of completed items during runtime completedIndex int + // lastPrinted tracks the last printed target during runtime lastPrinted string }
6-7: Remove commented-out code.The commented-out fields serve no functional purpose and should be removed. Version control preserves the history, so these lines can be safely deleted to reduce clutter.
Based on previous review feedback, the new field names are self-explanatory and don't require these commented lines as reference.
🔎 Proposed change
ResumeFrom string Index int - // current string - // currentIndex int completedIndex int lastPrinted string
3-10: Consider separating persistence format from runtime state.The
ResumeCfgstruct now serves two purposes:
- External persistence format (
ResumeFrom,Index- exported for serialization)- Internal runtime state (
completedIndex,lastPrinted- unexported)While the current approach maintains backward compatibility for the file format, mixing these concerns in a single struct can lead to confusion. The dual-field design (where
Indexis populated fromcompletedIndexandResumeFromfromlastPrinted) adds cognitive overhead.If backward compatibility with the existing
resume.cfgformat is a constraint, the current approach is acceptable. However, if you have the flexibility to refactor later, consider:
- A separate internal struct for runtime state tracking
- Clear separation between what's persisted vs. what's transient
For now, the unexported fields appropriately hide implementation details from external consumers.
Co-authored-by: Nakul Bharti <knakul853@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runner/runner.go (1)
1099-1103: Consider removing the extra blank line for consistency.The resume state update logic is correct and the comment is appropriately concise. However, line 1102 contains an unnecessary blank line that could be removed.
🔎 Proposed fix
// Update resume state after output to avoid skipping unprocessed targets on interrupt if r.options.resumeCfg != nil { r.options.resumeCfg.lastPrinted = resp.Input - }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runner/runner.go
🔇 Additional comments (1)
runner/runner.go (1)
2588-2593: Resume implementation is sound and no changes are needed.The SaveResumeConfig function correctly persists the runtime state, and the complete resume flow operates as intended:
- ✓ Resume loading properly initializes
IndexandResumeFromfrom resume.cfg viagoconfig.Load()- ✓
currentIndexcorrectly initializes to 0 on restart (default int value), while the loadedIndexvalue controls skip behavior- ✓ Skip logic works correctly: items are skipped while
currentIndex <= Index, resuming from the correct position- ✓ State tracking is accurate:
lastPrintedis updated after each output and correctly persisted asResumeFromThe change to use
lastPrintedinstead ofcurrentis consistent with the resume mechanism and poses no issues.
|
@ayuxsec thanks for resolving those comments. I tried to test and it seems the resume issue is still there, have you tested this? |
|
Hi @knakul853 I wasn’t able to understand how resume was parsed earlier (original dev’s intent), so I didn’t change any of the parsing by myself. I only made a partial fix to ensure the last successfully scanned host is written to |
2 participants
Partial Fix for #2345
before:
after:
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.