Skip to content

Report old requests discarded only after saving that state - #960

Open
robzolkos wants to merge 2 commits into
mainfrom
fix/handoff-discard-log-order
Open

robzolkos wants to merge 2 commits into
mainfrom
fix/handoff-discard-log-order

Conversation

@robzolkos

@robzolkos robzolkos commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Keep the narrow correctness fix from the retired restart-testing work: log that an old request was discarded only after the ledger write succeeds. A failed write previously left a success message in the log.

No new harness, timing knob, or change to the handoff age limit.

Verification

  • Added a focused test to the existing handoff tests. Its failed-save case fails before the fix and passes afterward; the successful-save control still records the discard and logs it.
  • Targeted handoff tests pass over 20 race-enabled runs.
  • Full local bin/ci passes.

Summary by cubic

PR Description

Fixes the handoff discard logging so a completion message is only emitted after the ledger write succeeds.

Previously, a request from before the run started was logged as "not handed off" before its discarded state was saved, so a failed save left a misleading success message in the log. Now the state is written first and the log line appears only if the write succeeds. Adds a test covering both save success and save failure.

Summary

Changes the handoff discard path so the discarded state is saved before the "not handed off" log line is written. A failed save now returns the error without logging a success message, and a new test verifies both the success and failure cases.

Written for commit ef7b223. Summary will update on new commits.

View guided diff Turn on auto-fix

@robzolkos
robzolkos requested a review from a team as a code owner October 23, 2026 20:07
Copilot AI balanced review requested due to automatic review settings October 23, 2026 20:07
@github-actions github-actions Bot added the tests Tests (unit and e2e) label Oct 23, 2026

Copilot AI 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.

🟢 Approved

The narrow fix preserves error propagation and handoff behavior, with regression coverage for both save outcomes and no unresolved findings.

No open issues 🎉

What changed in this PR

This PR fixes misleading connector logs by reporting an old request as discarded only after its ledger state is saved. It retains the narrow fix from #825 without changing the handoff age limit or adding testing infrastructure.

Changes:

  • Return ledger write errors before emitting the discard log.
  • Add regression coverage for successful and failed saves, confirming neither case hands off the old request.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
internal/​connector/​handoff.go Log discarded requests only after successful persistence.
internal/​connector/​handoff_test.go Test logging and output for both save outcomes.

Review effort: Balanced

@robzolkos

Copy link
Copy Markdown
Collaborator Author

Ready for review on ef7b223: local CI passes and all 31 hosted checks are accounted for (26 successful, 4 conditionally skipped, 1 neutral; none failed or pending). Copilot approved this exact head. There are 0 unresolved review threads and 0 unanswered findings. Cubic's fresh review is unavailable because its monthly quota is exhausted. Nothing merged.

@jeremy

jeremy commented Oct 23, 2026

Copy link
Copy Markdown
Member

🤖 Local review at ef7b223: clean. CI is green on it (27 passed, 4 skipped, 1 neutral).

Writing the discard first and logging after (handoff.go L246-L252) keeps the error return and prints nothing for the old request either way. I put the old order back and ran the new test: save_failed fails on the log line, so the test holds the fix. The local review ran go test ./internal/connector plus the two targeted tests (TestAnOldRequestIsLoggedOnlyAfterDiscardSucceeds, TestARequestFromBeforeTheRunIsNotHandedOff), and all passed. It didn't run the full suite.

#928 also edits the age check at the top of handOff, so whichever lands second will need a small rebase there.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants