Skip to content

fix(harness): cleanup_failed event + drop misleading exit_code capture - #2259

Merged
hongmingwang-moleculeai merged 1 commit into
stagingfrom
fix/harness-cleanup-failed-event
Apr 29, 2026
Merged

hongmingwang-moleculeai merged 1 commit into
stagingfrom
fix/harness-cleanup-failed-event

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Summary

Two self-review fixes on top of #2257 (harness hardening, just merged):

1. Drop misleading local exit_code=$? from cleanup()

trap-handler return values are ignored by bash, so capturing $? and returning it only misled a future reader into thinking exit-code preservation was happening. Removed the capture and the return $exit_code lines.

2. Surface cleanup failures as a structured event

The previous api -X DELETE ... >/dev/null 2>&1 swallowed every failure mode — including the realistic one (ADMIN_TOKEN expiring mid-run). When that happens, the workspace is leaked and there is no signal in the JSON event log.

Replaced with an -w '%{http_code}' capture:

  • 200 / 204cleanup_deleted event with the HTTP code
  • 404cleanup_deleted (post-condition holds — workspace absent — possibly a concurrent operator)
  • anything else → cleanup_failed event with a remediation hint pointing at cleanup-rogue-workspaces.sh

Test plan

  • bash -n passes
  • DRY_RUN=1 exits cleanly (cleanup runs but emits cleanup_skipped because IDs are still empty)
  • No behaviour change on the success path; only failure-path observability improves

Refs: #2257 review.

Self-review follow-ups on #2257:

- Drop `local exit_code=$?` from cleanup(). `trap`-handler return values
  are ignored, so capturing $? only misled a future reader into thinking
  exit-code preservation was happening.

- Replace silenced `>/dev/null 2>&1` DELETE with `-w '%{http_code}'`
  capture. ADMIN_TOKEN expiring mid-run was the realistic failure mode
  here — previously we swallowed it under the silenced redirect, leaving
  workspaces leaked with no signal. Now a 401/403/5xx surfaces as a
  `cleanup_failed` JSON event with a remediation hint pointing at
  cleanup-rogue-workspaces.sh; 404 is treated as success (the
  post-condition — workspace absent — holds).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged via the queue into staging with commit b78ca4a Apr 29, 2026
20 checks passed
HongmingWang-Rabbit pushed a commit that referenced this pull request Apr 29, 2026
Two docs covering load-bearing patterns from today's work that
weren't previously discoverable:

1. workspace/platform_tools/README.md — explains the ToolSpec
   single-source-of-truth pattern (#2240), the CLI-block alignment
   gap that hand-maintained generation can't close (#2258), the
   snapshot golden files + LF-pinning (#2260), and the add/rename/
   remove playbook. The next reader who lands in
   workspace/platform_tools/ now has the design rationale + the
   safe-edit procedure colocated with the code.

2. scripts/README.md — disambiguates the three measure-coordinator-
   task-bounds.sh files that now exist across two repos:

     - scripts/measure-coordinator-task-bounds.sh        (canonical OSS, this repo)
     - scripts/measure-coordinator-task-bounds-runner.sh (Hermes/MiniMax variant, this repo)
     - scripts/measure-coordinator-task-bounds.sh        (production-shape, in molecule-controlplane)

   Cross-references reference_harness_pair_pattern (auto-memory) for
   the cross-repo design rationale. Documents the common safety
   pattern (cleanup trap, DRY_RUN, non-target guard,
   cleanup_*_failed events) and the heartbeat-trace caveat.

Refs: #2240, #2254, #2257, #2258, #2259, #2260; molecule-controlplane#321.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
The status-pagination bug (RCA, #2440-family): merge/verify status readers
fetched only the FIRST page of a commit's statuses. On high-churn PRs Gitea
caps the combined GET /commits/{sha}/status `statuses` array at the default
page size (~30) and pushes older-but-still-current required-context rows past
it. A reader of that truncated view records the required context as ABSENT
(missing) even though its current SUCCESS row exists — wrongly blocking, or
mis-reading the gate. Confirmed on #2448/#2426/#2438/#2331/#2259/#2055/#2032
(reviewers had to manually paginate to verify gates this whole session). Live
proof on PR #2331 head: combined /status returns 30 rows; exhaustive
/statuses returns 50 rows across 20 distinct contexts.

Two verify-by-state readers consumed that capped combined view for
required-context decisions and are fixed here to page the dedicated
/commits/{sha}/statuses list to EXHAUSTION (until a short/empty page), then
collapse to newest-row-per-context:

- prod-auto-deploy.py (wait-ci gate): replaced the single combined /status
  fetch with fetch_all_statuses() (paginated). A required context past page 1
  no longer reads "missing" forever and times out a legitimate prod deploy.
  latest_status_for_context now selects newest-by-id so the oldest-first
  /statuses ordering can't let a stale run shadow the current one.
- audit-force-merge.sh: replaced the single combined /status fetch with a
  page loop over /commits/{sha}/statuses, accumulating all rows before the
  newest-wins CHECK_STATE collapse. A required SUCCESS past the cap no longer
  reads "missing" and emits a false-positive incident.force_merge.

gitea-merge-queue.py already paginates /statuses to exhaustion
(get_combined_status + api_paginated) — left unchanged; it is the reference
behavior this change brings the other two readers in line with.

STRENGTHENING ONLY — fail-closed preserved, NO fail-open path introduced:
- prod-auto-deploy: a genuinely-absent required context appears on NO page,
  so ci_context_state() still returns "missing", context_is_satisfied()
  rejects it, and the gate never greens (times out). Any page that errors or
  is not a list raises (fetch_all_statuses/_api_json_list) — a partial list
  never passes as complete.
- audit-force-merge: any non-200 page or non-array body aborts with exit 1;
  an absent required context has no CHECK_STATE entry so `${...:-missing}`
  keeps it not-green and the audit still fires.

Tests (mutation-resistant): added regressions that (a) place a required
SUCCESS on page 2+ behind a full page of churn and assert the reader FINDS it,
and (b) make a required context genuinely absent on all pages and assert the
reader STILL fail-closes (missing/never-satisfied → blocks/times out). Mocks
the paginated HTTP responses. Also locks newest-wins collapse, short-page
stop, full-page continue, and page-error propagation.

Refs: status-pagination RCA, #2440-family.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

1 participant