Skip to content

[WIP] Diagnose graceful restart drain behavior before next release - #78

Closed
aryanorastar with Copilot wants to merge 4 commits into
mainfrom
copilot/verify-graceful-restart-drain
Closed

aryanorastar with Copilot wants to merge 4 commits into
mainfrom
copilot/verify-graceful-restart-drain

Conversation

Copilot AI commented Oct 23, 2026 •

Copy link
Copy Markdown
  • Read AGENTS.md, ROADMAP.md, ARCHITECTURE.md, RUNBOOK.md, SUPPORT_MASTER_PLAN.md and issue Verify graceful restart drain before another release #66
  • Analyze index.js shutdown() and supportRuntime.js claims/leases/heartbeat/abandoned sweep/delivery fence
  • Code-side diagnosis: no runtime defect found in code; the evidence gap was absent per-case drain logs (bounded windows showed only the later abandoned-requeue line)
  • Add distinct, privacy-safe per-case shutdown outcome logging (completed during drain / returned unstarted / left for reclaim / delivery uncertain)
  • Red-before tests: 3 new tests FAIL on main and PASS on branch; 1 no-replay fence test passes on both
  • RUNBOOK.md: redeploy drain smoke steps + the log lines proving each outcome
  • ROADMAP.md honest status: code implemented / tests passed / rollout pending
  • npm test: 888/888 pass; EVAL_NO_MODEL=1 customer-eval: 8/8 ok; CodeQL: 0 alerts; secret scan clean; live-eval not run (spends model quota) — still needed before release
  • Final validation complete; working tree clean; both commits pushed to draft PR [WIP] Diagnose graceful restart drain behavior before next release #78

Cause named: no application defect found in code — drain already finishes started work within budget, returns unstarted work at timeout instead of leaving it abandoned, and never replays delivery-started work. The branch adds the missing per-case evidence. The live platform question (whether the old process had time to log during the manual Restart) is what the human-run redeploy smoke checks, per the new RUNBOOK section.

Copilot AI and others added 3 commits October 23, 2026 16:27
Co-authored-by: aryanorastar <106255197+aryanorastar@users.noreply.github.com>
Adds privacy-safe (internal message keys only) per-case drain logging so a
restart window distinguishes: completed during drain, returned unstarted,
left for reclaim with delivery not started, and left for reclaim with
uncertain delivery. Closes the issue-#66 evidence gap where a bounded log
window could not prove which outcome an in-flight request took.

Co-authored-by: aryanorastar <106255197+aryanorastar@users.noreply.github.com>
Adds a RUNBOOK section listing the per-case shutdown log lines that prove
each drain outcome and the human-run redeploy smoke steps, plus an honest
ROADMAP entry: code implemented and tests passed, rollout pending.

Co-authored-by: aryanorastar <106255197+aryanorastar@users.noreply.github.com>
Copilot AI force-pushed the copilot/verify-graceful-restart-drain branch from 404c2b4 to 5fdeb03 Compare October 23, 2026 16:30

Copy link
Copy Markdown
Collaborator

Integration update — rebased onto current main d108e1a

Main advanced while this PR was open: #67 (in-thread replies) and #74 (correction-link erasure) merged, and the controlled same-commit Redeploy drain smoke passed on the deployed main. This branch is rebased onto d108e1a and re-verified at head 5fdeb03.

What the rebase changed

Fresh verification at head 5fdeb03

  • npm test: 956/956 pass (main alone is 951; +5 = the four new drain tests plus one carried assertion count delta).
  • EVAL_NO_MODEL=1 node scripts/customer-eval.js: 8/8 ok.
  • Red-before reconfirmed: the 3 new drain tests FAIL against main's supportRuntime.js and PASS on the branch; the 4th pins the existing no-replay fence.
  • Secret scan clean; per-case log lines carry only internal message references (no customer text, Discord identities or order data).

Status / limits (unchanged): draft, unmerged, undeployed. Independent review and merge remain ahead; closing #66 stays with Aryan. Hosted Node 20/24/PostgreSQL CI on this exact head is not yet run — no CI workflow appears configured to run on this branch's push. live-eval not run (spends model quota).

@aryanorastar aryanorastar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Independent source review at5fdeb031: not merge-ready. (1) drain snapshots jobs before awaiting accepted tasks, so an already-accepted request whose database claim resolves during drain can be absent from the timeout deferral set. A synthetic delayed-claim probe ends queued on main but processing on this branch. (2) a rejected defer logs failure, then the next loop still logs returned-success based on !job.lost; a probe confirmed the row remains processing. Only confirmed outcomes may be logged; stale/zero-row owner updates also cannot establish requeue. (3) the runbook must not guarantee a later answer after deferral/database uncertainty or infer platform termination solely from missing logs. Require independent termination evidence. Repair in this same draft with deterministic delayed-claim/defer-failure/unknown-outcome tests and replace20/50ms setup sleeps with gates. The accepted controlled Redeploy smoke and closure of #66 remain valid; optional per-case diagnostics are not a renewed release gate. Review used source and tiny synthetic probes only, no production/customer/protected input or provider calls.

… runbook

Co-authored-by: aryanorastar <106255197+aryanorastar@users.noreply.github.com>

Copilot AI commented Oct 23, 2026

Copy link
Copy Markdown
Author

Independent source review at5fdeb031: not merge-ready. (1) drain snapshots jobs before awaiting accepted tasks, so an already-accepted request whose database claim resolves during drain can be absent ...

Addressed all three findings in 2458e9d:

(1) Late-claim gap — drain() now tracks every accepted job in a watchedJobs set added at the moment claim() succeeds (before any await), and reads that set after the budget expires rather than snapshotting this.jobs up front. A request whose DB claim resolves mid-drain is now returned/fenced instead of dropped. (I also removed a flawed 1000ms post-timeout grace I had tried — it was dead code and broke reservation release.)

(2) Unconfirmed returned log — defer() now returns whether the row actually re-queued (MemoryRuntimeStore: applied flag; PostgresRuntimeStore: rowCount > 0). The drain logs returned unstarted request only for confirmed returns; a rejected/zero-row/stale defer logs unstarted return failed and is never reported as success. job.lost is no longer used as a proxy for requeue success.

(3) Runbook certainty — the section no longer guarantees a later answer after deferral/DB uncertainty (recovery is described as the expected path to verify, not a promise) and no longer concludes platform termination from missing logs alone; it now requires independent termination evidence (deployment/instance state), noting bounded-window/scraping gaps as alternative causes.

Tests: replaced the 20/50ms setup sleeps with deterministic barriers. Added three red-before tests — delayed-claim tracking, defer-returns-false, defer-throws — all three FAIL on the pre-fix head 5fdeb03 and PASS now. Full npm test 959/959, offline eval 8/8, secret scan clean. Kept this draft; the accepted Redeploy smoke and #66 closure remain valid and this stays optional per-case diagnostics, not a renewed release gate.

@aryanorastar

Copy link
Copy Markdown
Collaborator

Closing as requested by the reviewer: #66’s goal was met by the controlled Redeploy smoke using existing logs. Per-case drain outcome logging, if needed, is retained under #51; the newer diagnostic work at 2458e9d is preserved on this branch, not merged or claimed deployed. The branch will not be deleted. The smoke proves clean drain for the observed scenario, not a universal exactly-once guarantee.

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.

Verify graceful restart drain before another release

2 participants