Resume benchmark fleet jobs after Spot interruptions

[laughingman7743]

WHAT

Benchmark fleet changes for Spot resilience and expiring results (#1136):

• Results location. New template parameters ResultsBucket (default empty) and ResultsPrefix (default pyathena-benchmark); the new Results stack output is s3://<ResultsBucket>/<ResultsPrefix>/<stack name>/, or s3://<scratch bucket>/ when ResultsBucket is empty. Fleet queues (fleet/<name>/), reports, and Athena trial outputs (runs/<run_id>/trials/) go there; the instance role can read and write objects only below that prefix and can list the whole results bucket (s3:ListBucket without a prefix condition, because S3 answers a read of a missing key with 403 instead of 404 without it, which breaks fsspec/pyarrow existence checks; the trade-off is that hosts can list key names of the whole results bucket). A stack from an earlier template without the Results output uses the scratch bucket. CTAS input snapshots, initialization fixtures, and Glue scratch tables stay in the scratch bucket and database. cleanup covers runs/<run_id>/ in both locations.
• Leased claims and slots. claims/<job> and slots/<n> carry attempt, state (running, interrupted, released), renewed, and expires; a thread renews them every --lease-seconds / 10 (600 s by default, at least 300) with IfMatch writes; a conditional write whose condition fails is re-read, so a retried write that already succeeded keeps its lease. A worker takes over an expired, interrupted, or released lease with an IfMatch write, so a dead host's job and slots return to the fleet; slots are released by overwriting the held lease, never by deleting the key. A worker whose claim is taken over or expires (including stalled renewals, detected locally) stops the attempt, quiesces its queries, and records nothing; if it cannot confirm the queries stopped, it raises and keeps its host. A lost slot is only recorded. The orchestrator runs in its own process group, killed when it ends, so no trial process outlives it. Before recording a job, a conditional claim renewal confirms ownership; the merged summary goes to job-summary.* of the publishing attempt and the first-wins done marker (IfNoneMatch) names it. Workers exit when nothing is pending; an expired claim of a dead worker is taken over by any running worker, and every lost path releases the claim conditionally, so a claim given up while still live returns to the fleet. status lists the claims of unfinished jobs and the slots, with host, attempt, state, and expired, and treats a report error as a failure.
• Interruption notices. A separate thread (errors logged, never fatal) polls IMDSv2 spot/instance-action (or the file interruption-notice in the worker's directory, for rehearsals) every 5 s. On a notice the worker sends SIGINT to the orchestrator (which stops the trial and cancels its reported queries), uploads the attempt (bounded waits), marks the claim interrupted, releases its slots, and exits without retiring the host. Rebalance recommendations are not acted on. The rehearsal file interruption-notice lives in the worker's directory, which the worker clears when it starts; after a rehearsal the operator terminates the host, as the README pilot does.
• Trial-level resume. Each attempt writes results/<job>/attempt-<n>/; the orchestrator (run --checkpoint) writes each trial's start before and its record after the trial to checkpoint/ in S3, so uploads never overlap a measured trial; the worker uploads the attempt's files once when it ends, and a fleet run writes no summary of its own. A takeover downloads the earlier attempts, quiesces their trial prefixes, and runs run --resume <earlier attempts>, which runs its own warmups and then only the measured repetitions not recorded as successful (and skips unsupported cases already recorded); a resume with nothing left exits 0. report merges attempt-* subdirectories, notes a failed or incomplete trial whose repetition succeeded in another attempt as Superseded: ... instead of counting it in failed_trials, and counts a repetition recorded twice once.
• Self-starting workers. With QueueName set, user data installs a systemd unit that runs worker --wait --terminate-host --api-slots <ApiSlots> for that queue, including on replacement hosts; the results location comes from user data (worker --results, also in results-location.txt), so workers need no stack outputs. --terminate-host now suspends AZRebalance when the worker starts instead of when it retires, so busy hosts are protected from the start. A host is kept by the instance tag KeepHost=true (read through IMDS; the launch template enables instance tags in metadata) instead of a local file.
• Spot default and zones. Purchase defaults to spot; new Zones parameter (4 by default for us-west-2, or 3; a three-zone region needs Zones=3) adds an optional fourth subnet.
• README: fleet, interruption, recovery, and cleanup procedures rewritten for the above, plus a one-host pilot with a simulated interruption.

queue already uploaded the configuration, manifest, and merged jobs; with the keep-host file gone, recovery and cleanup use fleet/<name>/manifest.json and no longer depend on the session host.

WHY

Fixes #1136. Confirmed by a one-host Spot pilot on AWS (see TEST).
In the 4.0.0 run (#644, #1130), two Spot interruptions needed manual slot deletion, manual worker starts on replacements, a retry queue that reran every repetition, manual AZRebalance suspension, and a kept session host. The maintainer's addendum on #1136 asks to write results to the expiring test bucket so that recovery and cleanup need fewer manual steps.

TEST

Tested commit: 0e5bbb7.

• just benchmark lint and just lint (includes cfn-lint): passed.
• just benchmark test: 181 passed, 1 skipped (after merging master with Benchmark read_sql_query and trim the next benchmark selection #1146 and Close asyncio cursor result sets in a thread #1142). New tests, grouped by object: TestLease (creation, takeover, stale-ETag writers, retried writes that already succeeded, ending after a concurrent renewal, slots written only when enough are free, downloads contained), TestJobWatch (renewal, claim taken over, stalled renewal past expiry), TestInterruptionNotice, TestRunJob (--checkpoint, --resume, SIGINT, process-group kill), TestWork (takeover of expired claims and slots, interruption and resume with equal trial counts, resume from a lost host's checkpoint only, notices during quiescing and between jobs, rehearsal file, lost claims, former owners, lost slots, unconfirmed queries, locally lost claims, failed summary uploads), TestWorkerCommand (--results without the stack), runner TestResume (results location, own warmups, checkpoints between trials), report merging and checkpoint reading, run_locations, cleanup of both locations, template parameters, IAM, and user data.
• One-host Spot pilot on AWS at 0e5bbb7d (stack pyathena-benchmark-pilot, r7g.2xlarge arm64, ResultsBucket=laughingman7743-pyathena; deleted afterwards, scratch database and bucket verified empty):
  • Queue pilot-1 (small, PandasCursor CSV, 6 repetitions per case): the systemd worker started by itself on the stack's queue, ran the job (51 s), wrote per-trial checkpoints and attempt-1/job-summary.* to the results location, wrote done (attempt 1, exit 0), recorded hosts/<id>.json with lifecycle: spot, suspended AZRebalance, and retired the host (desired capacity 1 → 0).
  • Queue pilot-2 (one case, 60 repetitions), published from the local machine with the manifest stored under fleet/pilot-1/: after the first checkpoints, the rehearsal notice file made the worker stop the running trial, mark the claim interrupted, and exit; the host was terminated without lowering the desired capacity. The replacement Spot host's worker started by itself and took over within about 1.5 minutes as attempt 2, ran only repetitions 6–59 (attempt 1 had 6 successful repetitions and the interrupted seventh), and wrote done (attempt 2, exit 0); attempt-2/job-summary.csv reports 60 successful trials, 0 failed, with the interrupted trial as a Superseded: interrupted note; the replacement host retired (desired capacity → 0).
  • Prefix-limited results access worked for Athena trial outputs, checkpoints, reports, and listing; cleanup --execute cleaned the run prefix in both the results location and the scratch bucket.
• Not run on AWS: AWS SDK for pandas (Wrangler) writes into the results location, and a real Spot interruption notice from IMDS (the rehearsal file exercises the same path after the notice is read).

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior): FINDINGS, repaired

Base d3d4c4df55cf5e64e143aaaf16c794e702bf87c6; initial pass on 1c75926f, repairs in 1dfcab19, record at head af75752b. Done by hand covering both the bug-hunting and the simplification perspectives; the /code-review and /simplify commands were not run because this session cannot start their subagents.

Covered: Queue leases (put, acquire, renew, read_lease, slot leases), JobWatch, pending_jobs, run_attempt, work, run_job, interruption/keep-tag/rebalance helpers, status; runner.run resume and trial output location; report.collect_trials; aws.split_location, stack_resources, cleanup; CLI wiring; template (results statements, user data unit, four-zone conditions, IMDS tags); README fleet/recovery sections; tests.

Findings repaired in 1dfcab19:

1. A notice that arrived while earlier attempts were quiesced still started the orchestrator (run_attempt); the attempt is now released as interrupted before running (test_a_notice_while_quiescing_an_earlier_attempt_skips_the_run).
2. The interrupted path did not upload the attempt's log; both paths now use upload_attempt().
3. Any report exception escaped before the done marker, so the lease would expire and another worker would rerun a finished job; report errors are now recorded as report_error in the marker.
4. README claimed replacements always continue jobs; that holds only with QueueName set.
5. run_job (Popen, --resume, SIGINT on the stop event) had no test; added with a fake process.

Checked, no change: the watch thread and the main thread share one boto3 S3 client (thread-safe); the interrupted path stops the watch before the final lease write, so the two threads never write the claim concurrently; quiesce of a failed job runs inside the watch, so a long quiesce keeps the claim alive; unsupported records are not duplicated across attempts; report merges only attempt-* subdirectories, so report results/ does not merge unrelated jobs.
Deferred: rebalance recommendations are recorded nowhere (documented as not acted on); --lease-seconds has an arbitrary 120 s floor.

[laughingman7743]

Round one, continued: /code-review (high) on 26748f7f, run by the coordinating session (relayed), and repairs in 3eefc13d

Each finding was verified against the code; all ten were accepted:

1. runner.py:395: a resumed attempt skipped warmups that ran on another host. → completed_trials() records only measured repetitions; each attempt runs its own warmups, then the missing measured repetitions; a case with every repetition recorded is skipped entirely (no wasted warmup). Test test_resumed_run_repeats_its_warmups_before_missing_repetitions.
2. fleet.py:1022: a lost attempt returned without quiescing, and a trial child could survive a SIGKILL. → the lost path runs settle(output) before returning, and quiesce errors stop the worker as after a failed job; run_job starts the orchestrator with start_new_session=True and kills its process group when it ends (SIGINT still goes to the orchestrator only). Tests: lost-claim test asserts the quiesce; run_job test asserts the group kill.
3. benchmark.yaml:317: prefix-conditioned s3:ListBucket makes S3 answer a read of a missing key with 403 instead of 404 (S3 HeadObject/GetObject docs: 404 only with s3:ListBucket; the condition has no s3:prefix for such requests), which breaks fsspec/pyarrow existence probes and also Queue.read_lease() on a missing claim. → s3:ListBucket on the results bucket without a condition (with GetBucketLocation, ListBucketMultipartUploads); object Get/Put/Delete stay below the stack's prefix. Trade-off, documented in the README: hosts can list key names of the whole results bucket (the CI test bucket holds only expiring test data).
4. fleet.py:826: a non-OSError (e.g. http.client.IncompleteRead) killed the notice thread or the worker loop. → interruption_notice() also catches http.client.HTTPException; check_notice() logs any other exception and returns False, used by the watch thread and work(). Test test_notice_errors_do_not_stop_the_watch_or_the_worker.
5. aws.py:80: older stacks without Results looped forever under --wait. → a missing Results output falls back to s3://<scratch bucket>/; IncompleteStackError (waited on) only while StackStatus ends with _IN_PROGRESS, otherwise a plain ValueError. Test test_stack_resources_waits_only_while_in_progress.
6. benchmark.yaml:34: default Zones=4. → kept 4 for the default region us-west-2 (four zones; in the 4.0.0 run two of three zones had no Spot capacity), and the README states that where Fn::GetAZs returns three zones the deployment needs Zones=3 and otherwise fails with a template error before creating resources.
7. report.py:185: superseded failures disappeared. → they stay in the merged trials marked superseded; failed_trials excludes them, the new superseded_failures column counts them, and their reasons appear in the notes as Superseded: .... Tests updated in test_report.py and the resume test.
8. fleet.py:952: whole growing JSONL files re-uploaded every 5 s. → uploads only at trial boundaries (a new trial_start in events.jsonl, or a new record in trials.jsonl), checked by reading only the bytes appended since the last check; renewals run before uploads in the loop. Test test_progress_uploads_at_trial_boundaries.
9. fleet.py:405: acquire() wrote before reading. → reads first; creates with IfNoneMatch only when absent, takes over with IfMatch only when takeable. Test test_acquire_does_not_write_a_live_lease.
10. __main__.py:253: a rehearsed notice left a healthy idle host. → decided not to retire (retiring would lower the desired capacity and prevent the replacement that the rehearsal tests); the worker removes the interruption-notice file when it starts, and the README tells the operator to terminate the host after a rehearsal, as the pilot does. Test test_worker_removes_a_rehearsal_notice_when_it_starts.

Self-review of the repairs (both perspectives): the read-first acquire depends on 404 for a missing key, which (3) now provides; the process-group kill runs after the orchestrator exits and ignores an empty group; a trial_start marker split across two polls is still uploaded at the trial's end; warmups are never marked superseded; the README and PR body claims match the code. Validation on 3eefc13d: just benchmark lint, just lint, just docs lint passed; just benchmark test 163 passed, 1 skipped.

[laughingman7743]

Round one, continued: merge of origin/master (#1146, #1142) and /simplify on b5795b5d, run by the coordinating session (relayed), applied in 493b8e95 (merge) and 7bbf1e14

Merge: conflicts were imports in __main__.py/test_fleet.py and new tests in test_report.py/test_runner.py (both sides kept). output_files() from #1146 lists the trial's prefix and reads the UNLOAD/CTAS manifests there; trial outputs are now in the results location, where the role lists the bucket and reads objects below the stack's prefix, so it works under the results-bucket grant.

Applied:

1. Checkpoints no longer overlap trials: run --checkpoint <attempt s3 prefix> writes checkpoint/<trial>-start.json before each supervise() and checkpoint/<trial>.json (the full record, needed for the merged summary after a lost host) after it, with put_object; the 5 s progress() uploads and upload_lock are gone; upload_attempt() uploads the files once. report.read_trials()/read_trial_starts() read trials.jsonl/events.jsonl plus the checkpoint, for resume, quiesce, and the merged summary. A run with --checkpoint writes no summary of its own (the --resume no-trials special case is removed).
2. User data writes results-location.txt and starts worker --results <location>; wait_for_stack, IncompleteStackError, the _IN_PROGRESS branch, and the fallback for stacks without Results are removed; --wait waits only for the queue.
3. The exit wait for other workers' claims is removed (Lease.stale, status stale, the waiting map, tests, README); a worker exits when nothing is pending.
4. Leases always carry attempt, state, renewed, expires; release_lease folded into end_lease(lease, "released"); state kept because status shows interrupted vs expired; the renew interval is lease_seconds / 10 inside JobWatch; the read-back recovery for retried conditional writes stays.
5. JobWatch sets one claim_lost event wherever loss is detected (renewal refused, renewal failing or stalling past the expiry), plus stop_event for the runner. run_attempt branches on interrupted / claim_lost / final renewal. Kept, after re-checking the Codex scenarios: the conditional renewal before publication (the IfNoneMatch done marker alone would let a stalled former owner record first while the new owner runs) and the expiry check before the marker (publication outlasting the renewed claim).
6. report.trial_key() used by runner.completed_trials() and the merge; superseded failures only as Superseded: ... notes (the superseded_failures column is removed).
7. aws.run_locations() for the cleanup preview, aws.cleanup, and the trial output location; local safe_settle(); METADATA_ERRORS; download_tree via download_file; Queue.put for jobs.json, host records, and tests; acquire_slots reads every slot and writes only when enough are free (Queue.take() reuses the read).
8. The rehearsal file is interruption-notice in the worker's directory; work() removes it when it starts, next to the default notice that reads it.
9. Tests regrouped as TestLease, TestJobWatch, TestInterruptionNotice, TestRunJob, TestWork, TestWorkerCommand, runner TestResume, report tests in TestReport; helpers take_over() and recording_runner(). New: resume from a lost host's checkpoint only, checkpoints written between trials, slots written only when enough are free, worker using --results without the stack.

Skipped as requested: IMDS token caching; Zones (default 4, 3 as an option).

Self-review of the repair (both perspectives): checkpoint writes cannot overlap a measured trial (before supervise() / after the record); a SIGINT during a checkpoint write leaves the local record, which upload_attempt() uploads; a notice racing a successful run releases the claim as interrupted and the takeover finds every trial checkpointed, runs none, and records the job; the lost-claim path still quiesces and keeps the host on unconfirmed queries; README claims re-read (pilot paths: rehearsal file under results/fleet/pilot-1/, checkpoint location). Validation on 7bbf1e14: just benchmark lint, just lint, just docs lint passed; just benchmark test 178 passed, 1 skipped.

[laughingman7743]

Self-review round two (claims, callers, operations): FINDINGS, repaired in af75752b

Base d3d4c4df55cf5e64e143aaaf16c794e702bf87c6, head af75752b77b99a7a9abf1ed680a7642d08e81253.

• AWS operator, retries (repaired): botocore retries a PutObject whose response was lost; the retry of a successful conditional write fails its precondition. renew would then drop a running job's lease (another worker could take over and duplicate the job), and acquire would skip a claim it created (the job idles until the lease expires). Queue.put_lease() now re-reads after a failed condition and keeps the lease when the object holds exactly the written body (bodies are unique through their float expiry). Test test_a_retried_write_that_already_succeeded_keeps_the_lease (create, takeover, renew) fails on 1dfcab19.
• Claims checked: IMDS spot/instance-action exists only after an interruption notice, about two minutes ahead (EC2 Spot docs); instance tags in metadata (InstanceMetadataTags: enabled) reflect tags changed while running; PutObject IfMatch/IfNoneMatch on general purpose buckets return 412 PreconditionFailed or 409 ConditionalRequestConflict; botocore 1.43.102 in uv.lock has IfMatch; the CI bucket rule is ExpirationInDays: 1 (expiry at the following midnight UTC). Wording corrected: rebalance recommendations are not always followed by an interruption; expiry timing; what a lost host loses; the pilot criterion covers every case of the job; PR body on AZRebalance (now suspended at worker start only).
• Existing callers: Queue(s3, bucket, name) keeps working (base defaults to empty); stack_resources now requires the Results output, so stacks deployed from older templates are rejected (disposable stacks); status changes slots_in_use from a list to a mapping; retire_host no longer takes keep. Only this package calls these.
• Adversarial: clock skew between hosts only shifts takeover by the skew (EC2 time sync); a worker whose lease was taken over keeps running its attempt and records lost_leases in its done marker — the later done write wins; two takeovers race through IfMatch, one wins.
• Documentation reader: README pilot and recovery commands walked through against the CLI options (jobs --warmups/--repetitions, queue --manifest, worker --wait/--lease-seconds, report <job dir>, cleanup on the queued manifest).
• Evidence: all tests use fake S3/Auto Scaling/IMDS; no AWS stack was deployed. IAM scoping of Athena and Wrangler writes under a prefix-conditioned results bucket, the systemd unit, IMDS tags, and replacement takeover remain for the README pilot.

[laughingman7743]

AWS pilot (evidence for the operational claims) on 0e5bbb7d — PASSED

• Stack pyathena-benchmark-pilot (1 Spot r7g.2xlarge in us-west-2b, ResultsBucket=laughingman7743-pyathena, QueueName=pilot-1, later updated to pilot-2). Evidence recovered locally (352 files incl. stack.json), then cleanup --execute, scratch database/bucket verified empty, stack deleted; no instances or stacks remain.
• pilot-1: self-started worker, per-trial checkpoints, merged summary, done attempt 1 exit 0, host record lifecycle: spot, AZRebalance suspended, desired capacity 1 → 0.
• pilot-2 (one case, 60 repetitions, queued from the local machine with the stored manifest): rehearsal notice → claim interrupted; host terminated without decrement; replacement self-started worker took over in ~1.5 min as attempt 2 and ran repetitions 6–59 only (attempt 1: repetitions 0–5 ok, 6 interrupted); done attempt 2 exit 0; summary 60 ok / 0 failed with Superseded: interrupted; desired capacity → 0.
• Not exercised: Wrangler writes into the results location; a real IMDS notice (the rehearsal file takes the same path after the read).

[laughingman7743]

Independent review (relayed Codex result): FINDINGS (9)

• Reviewer: Codex CLI 0.162.1, codex exec --sandbox read-only --ephemeral, thread 01a12ecc-7b6a-71f1-8dba-14b6a6f0bc74; static review, no edits, builds, tests, AWS commands, or GitHub access. Base d3d4c4df55cf5e64e143aaaf16c794e702bf87c6, head af75752b77b99a7a9abf1ed680a7642d08e81253, frozen detached worktree (unchanged afterwards); prompt without the PR number, description, commit messages, or self-review findings.

Reviewer output, verbatim except local paths:

Covered all 12 changed files: results routing and cleanup, leases and SDK retries, interruption handling, resume/report merging, IAM, CloudFormation conditions/user data/fourth-zone ordering, host retirement, README claims, and test coverage. This was static inspection; no edits, builds, tests, AWS commands, or GitHub access.

FINDINGS

1. P1 — Bootstrap workers can exit before stack outputs exist. benchmarks/cloudformation/benchmark.yaml:391
   An early host starts its service while CloudFormation still waits for other hosts’ creation signals. The worker immediately calls stack_resources(), which raises “Stack outputs are incomplete”; --wait only applies after that lookup. The unit has no restart policy, so publishing the queue later leaves these hosts idle. Wait/retry output discovery during bootstrap. AWS confirms outputs are unavailable during IN_PROGRESS states. CloudFormation documentation

2. P1 — Losing a lease does not stop the previous owner. benchmarks/pyathena_bench/fleet.py:750
   If A’s renewal stalls until B takes over, A merely appends the lease to lost. Its orchestrator continues running, and A can subsequently overwrite the shared summary and done/<job> marker. Renewal exceptions likewise never enforce the local expiry deadline. Stop work when ownership expires or is lost, and prevent stale owners from publishing completion.

3. P1 — Slot release can delete another worker’s lease. benchmarks/pyathena_bench/fleet.py:1015
   After A’s slot expires and B conditionally takes it over, A’s finally block unconditionally deletes that key. C can then acquire it while B is still paging, exceeding --api-slots. The partial-acquisition cleanup has the same problem. Release using the held lease’s ETag; deleting by key discards the ownership protection used during acquisition.

4. P1 — A replacement can retire before the dead worker’s claim expires. benchmarks/pyathena_bench/fleet.py:970
   With one unfinished job, suppose its host disappears immediately after renewing its 600-second claim. A replacement that finishes bootstrap before expiry sees no pending jobs, returns normally, and terminates itself with desired-capacity decrement. The fleet reaches zero hosts with an unfinished job. Retirement must distinguish completed jobs from temporarily unavailable claims.

5. P2 — Blocking S3 operations can consume the entire interruption window. benchmarks/pyathena_bench/fleet.py:757
   Metadata polling, sequential lease renewals, and progress uploads share one thread. The configured 60-second read timeout plus SDK retries allows one S3 operation to block beyond the two-minute Spot notice window. During that interval no notice is checked; watch.stop() also joins without a deadline. A host can therefore disappear without SIGINT or interrupted-claim publication. Separate interruption polling from blocking transfers and bound shutdown work.

6. P2 — An older checkpoint can overwrite the final trial records. benchmarks/pyathena_bench/fleet.py:872
   Normal completion uploads the attempt while JobWatch can still upload the same JSONL keys. A checkpoint captures a shorter file, stalls, and completes or retries after the final upload, replacing it with fewer records. The worker can still publish a complete summary and successful done marker. Serialize checkpoint and final uploads while maintaining lease renewal.

7. P2 — Resuming an already-complete attempt reports failure. benchmarks/pyathena_bench/__main__.py:296
   An interruption after the last successful repetition but before completion publication leaves every repetition available for resume. The replacement skips them all and run() returns success, but its new directory has no trial records. report(args.out) raises “No trials to report”, causing exit code 1 and a failed fleet marker despite a complete merged report. Handle a resume requiring zero additional trials as successful.

8. P2 — Recovery cannot quiesce trials missing from the last checkpoint. benchmarks/pyathena_bench/fleet.py:840
   If a host dies after submitting a long Athena query but before its first progress upload, no earlier attempt is downloaded and no recovery quiescence runs. The query’s random output prefix existed only in local events.jsonl; a replacement can start measurements while that query remains active. Persist the attempt’s query namespace before submission, or otherwise make uncheckpointed queries discoverable.

9. P2 — The results-bucket grant blocks multipart cleanup. benchmarks/cloudformation/benchmark.yaml:312
s3:ListBucketMultipartUploads does not support the s3:prefix condition key. Consequently, cleanup under the instance role fails at aws.py:429, even though the request supplies Prefix, before deleting results objects. Grant this bucket-level operation separately while retaining prefix-scoped object permissions. The same strict condition also excludes prefixless HeadBucket requests. S3 authorization reference, HeadBucket permissions

The tests can pass with these defects: CloudFormation tests assert template text and policy shape; the lost-lease test only checks watch.lost; fleet resume tests inject fabricated successful records; runner tests bypass the CLI reporting step; and the S3 fakes omit IAM enforcement and delayed concurrent uploads/deletes.

[laughingman7743]

Repairs of the independent review (4b06a6c3, e033f454) — all nine findings verified:

1. worker --wait retries stack_resources() while outputs are incomplete (IncompleteStackError); test test_worker_waits_for_stack_outputs.
2. A lost claim stops the attempt (SIGINT) and publishes nothing; a final conditional renewal preceded the done marker (superseded below).
3. Slots are released by overwriting the held lease (release_lease), not by deleting the key; test test_releasing_an_expired_slot_keeps_another_workers_takeover.
4. Workers did not exit while a claim looked stale (superseded below).
5. A separate notice thread; on a notice the claim was released first (reordered below).
6. Checkpoint and final uploads serialized by upload_lock.
7. A resumed run with no trials left exits 0; test test_a_resumed_run_without_remaining_trials_succeeds.
8. Changed .jsonl files are checkpointed every 5 s; the residual window (a trial started within seconds before a silent host death) is documented — its queries write to their own trial prefix and cleanup cancels them.
9. s3:ListBucketMultipartUploads granted without the s3:prefix condition, which it does not support.

Independent follow-up 1 (relayed Codex result) on e033f4547ea0795975da1cc79ec5c3489efad69e, thread 01a12ee4-8d2e-7861-b8ce-9324f0700b6f, frozen snapshot unchanged, static review:

Covered af75752b..e033f4547ea0795975da1cc79ec5c3489efad69e: stack bootstrap, conditional writes/retries, shared leases, watcher shutdown, stale-claim retirement, interruption/checkpoint ordering, resume/reporting, IAM, and regression tests. Both supplied diffs match the snapshot. Static inspection only; no edits, builds, tests, AWS commands, or GitHub access.

FINDINGS

1. P1 — Final publication can still occur after ownership is lost. benchmarks/pyathena_bench/fleet.py:991
   The watcher has stopped before the final renewal, report generation, summary uploads, and unconditional done write. With the supported 120-second lease, a slow summary upload can outlast that renewal. Another worker can take over and publish its result, after which the old worker overwrites the summaries and done marker. The single renewal does not protect the subsequent publication window.

2. P1 — A replacement can still retire before a dead claim becomes stale. benchmarks/pyathena_bench/fleet.py:1093
   A 600-second claim is considered stale only after 120 seconds without renewal. If its owner dies immediately after renewing and the replacement reaches this check within those 120 seconds, pending is empty and stale is false. The replacement returns normally and decrements desired capacity, leaving the unfinished job without a worker. No observed heartbeat advance establishes that the purported live owner actually survived.

3. P1 — Blocking S3 work prevents detection of lease expiry. benchmarks/pyathena_bench/fleet.py:833
   Expiry is checked only after a renewal raises; the same thread first performs checkpoint uploads. An upload or renewal blocked beyond the lease deadline leaves stop_requested unset while another host takes over. The independent notice thread checks only Spot notices. This is reachable with a 120-second lease and the configured 60-second read timeout plus retries. Boto3 retry semantics

4. P1 — Losing only a slot abandons a claim the worker still owns. benchmarks/pyathena_bench/fleet.py:977
   If the claim renews successfully but an API slot expires or is taken over, the attempt returns lost without releasing its claim or calling settle(output). The worker can continue to another job with unconfirmed queries outstanding. When other work finishes, its still-fresh claim can also trigger normal retirement, leaving this job unfinished. Slot loss needs handling distinct from another worker owning the job claim.

5. P2 — Slot release can fail against the worker’s own in-flight renewal. benchmarks/pyathena_bench/fleet.py:445
   Suppose S3 accepts a slot renewal but delays its response. The bounded join returns while the shared Lease still contains the old ETag. release_lease() then fails its conditional write, and benchmarks/pyathena_bench/fleet.py:1138 ignores the false result. The exiting host leaves its renewed slot occupied until expiry. Unlike mark_interrupted(), slot release does not reconcile a renewal by the same owner. S3 conditional-write behavior

6. P2 — Interruption exposes the claim before its final records are available. benchmarks/pyathena_bench/fleet.py:982
   For a job without API slots, another worker can acquire the interrupted claim immediately and download the previous checkpoint before upload_attempt() finishes. It then repeats successes missing from that checkpoint. The old worker subsequently uploads those successes, and collect_trials() retains both successful records for the same repetition. Reporting all downloaded attempts therefore double-counts repetitions; the new worker’s cached report can also differ from the archived evidence.

7. P2 — The bounded join is followed by an unbounded upload wait. benchmarks/pyathena_bench/fleet.py:953
   If the checkpoint thread holds upload_lock inside a slow upload, stop(timeout=10) returns but the interruption path immediately waits on that same lock through upload_attempt(). Slot release remains after this wait. EC2 can reclaim the host before shutdown reaches slot release or the final checkpoint, despite the bounded joins.

The stack-output wait (#1), protection against deleting another owner’s slot (#3), checkpoint/final-upload serialization (#6), empty-resume exit handling (#7), and IAM action split (#9) look correct in isolation. The residual checkpoint window (#8) is documented. The IAM split agrees with AWS’s supported S3 condition keys.

By static inspection, the added tests would reject the corresponding old behavior for takeover-safe release, detected claim loss, already-stale waiting, stack-output waiting, empty resume, and the IAM policy. Their limits are material:

• benchmarks/tests/test_fleet.py:885 performs sequential calls on separate Lease objects; it does not exercise concurrent mutation or delayed responses.
• benchmarks/tests/test_fleet.py:924 begins with an already-stale claim, missing the fresh-dead-claim exit.
• No added test covers blocked-S3 expiry, publication after watcher shutdown, slot-only loss, overlapping handoff/upload, or bounded shutdown.
• The existing progress test uses a 0.01-second renewal interval, so the previous upload-at-renewal implementation also satisfies it; it does not protect the new five-second checkpoint cadence.

Repairs (6531dbf0, 9e349def), all seven verified:

1. The done marker is created with IfNoneMatch through put_lease (a retried write that succeeded is recognized); the watch keeps renewing through publication; only the worker that created the marker uploads the summary; a claim lost or locally expired after the run returns lost before publication; --lease-seconds ≥ 300. Test test_an_existing_done_marker_is_not_replaced.
2. Exit rule: a worker exits only after every other unfinished claim's renewed time changed since it found nothing to claim; a dead claim is never renewed and is taken over at expiry. Test test_a_fresh_claim_that_is_never_renewed_is_taken_over_after_it_expires; the live-claim tests now renew while the worker waits.
3. The notice thread also checks the claim's local expiry; test test_watch_stops_the_job_when_renewals_fail_until_the_claim_expires.
4. Slot loss no longer stops the job; done lists lost_slots; test test_losing_a_slot_does_not_stop_the_job.
5. end_lease() reconciles with the same owner's current object for both released and interrupted; test extended.
6. On a notice: upload (bounded lock wait), then release the claim as interrupted; collect_trials() keeps one successful record per repetition across attempts; test test_report_counts_a_repetition_recorded_by_two_attempts_once.
7. Upload lock waits bounded to 10 s on the interruption path.

Validation on 9e349def: just benchmark lint, just lint, just docs lint passed; just benchmark test 153 passed, 1 skipped. Self-review of the repairs found one more gap (a claim that expired during settle/report set stop_requested without being in lost), repaired in 9e349def.

[laughingman7743]

Independent follow-up 2 (relayed Codex result) on 9e349def875e880902a19826fe9162c2a7846497, thread 01a12f04-20cb-7742-bbd2-9c1271e664d8, static review, snapshot unchanged:

Covered: follow-up diff through 9e349def875e880902a19826fe9162c2a7846497, worker retirement, publication and conditional writes, shared Lease interactions, interruption/upload locks, report deduplication, and regression-test sensitivity. Static review only; no files changed or prohibited operations performed.

FINDINGS

1. P1 — A former owner can still publish first. benchmarks/pyathena_bench/fleet.py:1012
   Publication now checks only asynchronous watcher flags; the synchronous ownership check was removed. If A stalls, B takes its expired claim, and A resumes before its watcher detects the takeover, A can create done while B is still working. A failed outcome then permanently prevents B’s successful outcome from being published. IfNoneMatch protects an existing marker but does not establish claim ownership. Completion needs a conditional transition tied to the currently owned claim.

2. P2 — An observed renewal remains acceptable after becoming stale. benchmarks/pyathena_bench/fleet.py:1124
   With 600-second leases, an idle worker can start waiting for A and B at t=0, observe A renew at t=60 and subsequently die, then keep waiting because B’s renewal is stalled. If B publishes completion with quiesce errors at t=240 and exits, A’s renewal still differs from the original baseline. The idle worker therefore retires despite A already being stale, leaving no worker to recover A. Recheck freshness when deciding to exit.

3. P2 — Completion is committed before summary publication succeeds. benchmarks/pyathena_bench/fleet.py:1035
   A crash or upload exception after creating done leaves a successful marker with report_error=None but missing summaries. Every subsequent worker skips that job, so automatic recovery cannot finish publication. Stage the merged reports before committing completion, or provide recovery for incomplete publication.

4. P2 — The expiry regression test passes without the expiry-watch fix. benchmarks/tests/test_fleet.py:1011
   The mock raises immediately. The previous _renew loop already detects expiry after such exceptions and sets stop_requested, satisfying this test. A blocked renewal or checkpoint—the reported failure scenario—never reaches that handler. Block the operation with synchronization and require expiry detection before unblocking it.

Repairs (0f85514d), all four verified: synchronous conditional claim renewal after the watch stops and before publication; summaries before the done marker; exit requires renewed-and-fresh claims (test test_a_claim_renewed_once_then_stale_does_not_let_the_worker_exit); the expiry test blocks a renewal; the former-owner test (test_a_former_owner_records_nothing_after_a_takeover) replaces the done-marker test whose scenario can no longer occur.

Independent follow-up 3 (relayed Codex result) on 0f85514d371120f296bde8f3d14be1567837a7ac, static review, snapshot unchanged:

Covered surfaces: 9e349def → 0f85514d371120f296bde8f3d14be1567837a7ac, including claim renewal, publication, worker exit, watch threads, changed tests, and CLI/CloudFormation retirement paths. Both supplied diffs match the snapshot.

FINDINGS

1. P2 — Publication can still continue after ownership expires. benchmarks/pyathena_bench/fleet.py:1017
   The watch is stopped before renewal, and no ownership check follows the summary uploads. Queue.renew() calculates expiry before its S3 request, so successful renewal does not guarantee 300 seconds remain. If renewal/upload retries consume the lease, another worker can take over and finish. The former owner can then overwrite that worker’s summaries before its done-marker write is rejected—or publish the first done marker while the replacement is running. Publication needs protection against ownership loss throughout this interval.

2. P2 — A claim expiring during the scan can still let the last worker retire. benchmarks/pyathena_bench/fleet.py:1132
pending_jobs() evaluates expiry using a timestamp captured before reading claims. The exit decision uses a later timestamp, but Lease.stale() explicitly returns false for expired claims. If a previously renewed claim expires during the scan, it remains in running and satisfies not lease.stale(at). The worker can therefore exit and terminate with that job unfinished. Recheck not lease.takeable(at) alongside freshness at the exit decision.

3. P2 — The expiry regression can still pass without the expiry-watch fix. benchmarks/tests/test_fleet.py:1015
   Both waits use five seconds. When the renewal’s wait starts first, it can time out before the main stop assertion. Its AssertionError is caught by _renew(), which observes the expired lease and sets stop_requested. Thus the test can pass with the expiry branch removed from _watch_notice(). Keep renewal blocked until explicit cleanup, using finally to release it after the stop assertion.

The summary-before-done ordering is corrected, but no changed test detects reverting that ordering. The former-owner test detects removal of the final renewal; the stale-claim test detects removal of the freshness guard under its intended timing, although oversleeping its 100 ms window can bypass that check.

Static review only; no edits, builds, tests, AWS commands, or GitHub access.

Repairs (f0dc49f5), all three verified: the done marker is not created after the claim's local expiry (residual: summaries uploaded just before an unnoticed expiry could precede a replacement's own later summaries, which then overwrite them); the exit decision also requires not takeable(at); the stalled-renewal test keeps the renewal blocked until cleanup; the resume test asserts summary-before-done. Validation on f0dc49f5: just benchmark lint, just lint, just docs lint passed; just benchmark test 154 passed, 1 skipped.

[laughingman7743]

Independent follow-up 4 (relayed Codex result) on f0dc49f5, static review, snapshot unchanged:

Covered surfaces: 0f85514d → f0dc49f5, including publication, claim expiry, worker exit, watch cleanup, changed tests, resume/report merging, and CLI/CloudFormation retirement paths. Both supplied diffs match the snapshot.

FINDINGS

1. P2 — An expired publisher can still overwrite the replacement’s completed results. benchmarks/pyathena_bench/fleet.py:1027
   The expiry check runs after unconditional uploads to the shared summary keys. Worker A’s upload can stall across expiry; B then takes over, successfully reruns A’s failed trial, and publishes its summaries and done marker. A’s delayed upload subsequently overwrites B’s summaries before this check returns "lost". The queue remains done, but its published report contains A’s failures or missing trials. The replacement’s later start does not ensure its uploads finish last. Publication must prevent stale attempts from overwriting the completed result.

The scan-expiry retirement defect is resolved by benchmarks/pyathena_bench/fleet.py:1138.

Static test assessment:

• benchmarks/tests/test_fleet.py:794 would fail if summary/done ordering were reversed.
• benchmarks/tests/test_fleet.py:1035 no longer falsely passes without the expiry watch. However, removing that branch makes the assertion time out, then __exit__() blocks joining the renewal thread before the outer finally releases it. Cleanup needs an inner finally inside with watch to produce a normal test failure.
• Neither new production guard has a deterministic regression test covering its expiry boundary.

Static review only; no edits, builds, tests, AWS commands, or GitHub access. HEAD and worktree remain unchanged.

Repair (26748f7f): the merged job summary goes to results/<job>/attempt-<n>/job-summary.{csv,md} of the publishing attempt, and the first-wins done marker names it under summary, so a former owner's late upload cannot replace the recorded summary; the stalled-renewal test releases the renewal in an inner finally. Validation on 26748f7f: just benchmark lint, just lint, just docs lint passed; just benchmark test 154 passed, 1 skipped.

Independent follow-up 5 (relayed Codex result) on 26748f7fae29e72d17ff514216f5d6a6b2166dd7, thread 01a12f30-830c-7f12-8e61-a345fcd93987, static review, snapshot unchanged: CLEAN

Covered f0dc49f5 → 26748f7f: publication races, attempt allocation, done-marker ownership, recovery/report consumers, S3 permissions, renewal-test cleanup, and README changes. Both supplied diffs match Git.

CLEAN — both previous findings are resolved; no actionable defects found within the requested scope.

• benchmarks/pyathena_bench/fleet.py:1023: summaries use attempt-specific keys, and the first-wins done marker identifies that attempt. An expired publisher’s late upload cannot overwrite the replacement’s recorded summaries.
• benchmarks/tests/test_fleet.py:1033: the inner finally releases the stalled renewal before context-manager cleanup joins the thread, including when an assertion fails.

Static inspection only. No edits, builds, tests, AWS commands, or GitHub access.

[laughingman7743]

Independent follow-up 6 (relayed Codex result) on 3eefc13d (the /code-review repairs), thread 01a12f44-5454-7e21-951c-d20f57141c10, static review, snapshot unchanged:

Covered: 26748f7f..3eefc13d, including warmups/resume, claim cleanup, process groups, notices/rehearsal, checkpoint uploads, reports, stack waiting/fallback, results IAM, Zones, and changed tests. Both supplied diffs match Git exactly. Static review only; no edits, builds, tests, AWS commands, or GitHub access.

FINDINGS

1. P2 — Failed cleanup of a lost attempt can terminate the last worker with unfinished work. benchmarks/pyathena_bench/fleet.py:1215
   With one worker, let its claim expire without a takeover, then let quiescence fail. run_attempt() returns Outcome("lost", errors) without a done marker, and this branch returns normally. The benchmarks/pyathena_bench/__main__.py:261 consequently terminates the host while decrementing desired capacity to zero. The unfinished job has no replacement worker. Preserve replacement capacity when exiting because an unfinished attempt could not be quiesced. The modified lost-claim test only exercises successful quiescence.

2. P2 — Failed boundary uploads are forgotten instead of retried. benchmarks/pyathena_bench/fleet.py:977
   Both checked counters advance before uploading. If a trial-start upload fails transiently, subsequent polls see no new boundary and never retry it until another trial boundary occurs. If the host dies during that long trial, its replacement lacks the trial prefix and cannot quiesce its surviving Athena queries; resumed measurements can overlap those queries and record contaminated timings. Keep the boundary pending until uploads succeed. The new upload test uses exclusively successful uploads.

By source inspection, the added assertions distinguish the earlier implementation for the main fixes: warmups, quiescence invocation, process-group handling, IAM listing, stack fallback/status handling, superseded failures, upload frequency, read-before-write, and rehearsal-file removal. Coverage does not directly establish notice-thread exception survival, forced-kill cleanup, renewal-before-upload ordering, or either failure scenario above.

For benchmarks/README.md:101, the four-zone default and invalid fourth selection with three AZs are confirmed. AWS documents an out-of-range selection as a stack error; I could not substantiate the stronger “before it creates any resource” guarantee. The tests inspect template structure and do not establish that timing.

Repairs (ae77dab7): a lost attempt whose quiesce fails now raises RuntimeError, so the worker keeps its host instead of retiring it with a lower desired capacity (test test_an_unconfirmed_stopped_attempt_keeps_the_host); a trial boundary stays pending until its upload succeeds (test test_a_failed_trial_start_upload_is_retried); README now says only that a three-zone deployment with the default Zones fails with a stack error.

Independent follow-up 7 (relayed Codex result) on ae77dab737c27daaba4032c2de604cc45fa4fa54, thread 01a12f4e-5ec0-7813-9f40-18664c090b75:

Covered surfaces: 3eefc13d → ae77dab737c2; lost-claim cleanup and outcome propagation, CLI retirement, slot release, boundary-upload retries, README zone wording, and both new tests.

FINDINGS

• P2 — Cleanup errors still disappear on late claim loss. benchmarks/pyathena_bench/fleet.py:1217: The new guard depends on outcome.errors, but the late return at benchmarks/pyathena_bench/fleet.py:1056 discards collected errors. If a failed runner enters cleanup before losing its claim, renewal failures can expire that claim while cleanup waits and ultimately reports still RUNNING. This returns an error-free lost outcome. With another worker holding that job and another job pending, this worker starts further trials despite the surviving query, contaminating recorded measurements. Preserve cleanup errors through the late lost returns and cover claim loss during cleanup/publication. Repair (1) remains incomplete.

The pending-upload fix and README correction look correct.

By static inspection, both new tests would fail without their respective fixes:

• benchmarks/tests/test_fleet.py:1252 expects the exception where the old worker returned normally.
• benchmarks/tests/test_fleet.py:1285 requires the retry before the runner returns, preventing the final upload from masking the defect.

The first test does not cover the late-loss scenario above. No files changed, builds/tests run, or AWS/GitHub access performed.

Repair (b5795b5d): every late lost return carries the collected quiesce errors, so the worker raises and keeps its host; test test_cleanup_errors_survive_a_claim_lost_during_cleanup.

Independent follow-up 8 (relayed Codex result) on b5795b5daeeff377ff47cc59c83d5019df9eea6f, thread 01a12f52-f80f-7df2-9a34-0c84657461c0, static review, snapshot unchanged: CLEAN

Covered: the three changed late-loss returns, cleanup-error propagation through work(), slot release, host-retirement control flow, and the added regression test at snapshot b5795b5daeeff377ff47cc59c83d5019df9eea6f.

CLEAN — the previous defect is resolved; no new actionable defects found within the requested scope.

• benchmarks/pyathena_bench/fleet.py:1056, lines 1056, 1066, and 1084, now preserve cleanup errors across failed ownership renewal, publication expiry, and done-marker conflict. These errors trigger the existing worker exception, preventing further trials and bypassing host retirement.
• By static tracing, the benchmarks/tests/test_fleet.py:1292 would fail against the previous head: job a loses its errors, job b runs and records a done marker, and pytest.raises fails because no exception occurs. It directly covers the cleanup/renewal branch; the other two changed branches are not independently exercised by this test.

Static inspection only. No files edited, builds/tests run, or AWS/GitHub access performed.

Validation on b5795b5d: just benchmark lint, just lint, just docs lint passed; just benchmark test 166 passed, 1 skipped.

[laughingman7743]

Independent review of the merge and /simplify (relayed Codex result): full pass of the changed areas on 7bbf1e14, thread 01a12f69-1677-7770-94cc-442a0cc3718e, static review, frozen snapshot unchanged:

Covered: fleet leases, slots, watch threads, attempts, worker lifecycle, notices and status; runner checkpoints/resumption; report merging; results locations and cleanup; CLI wiring; CloudFormation user data/IAM; README fleet/pilot/recovery procedures; associated tests. Static review only; no edits, builds, tests, AWS commands, or GitHub access.

FINDINGS

1. P1 — An abandoned claim can cause the last worker to retire with unfinished work. benchmarks/pyathena_bench/fleet.py:994
   S3 can accept a renewal while its response stalls beyond the locally known expiry. The watch then sets claim_lost, but the remote claim remains live—even if the renewal eventually returns successfully. This branch abandons it without ending it. On the next scan, pending_jobs() excludes that live claim, so the sole worker returns normally and retires its host, reducing desired capacity to zero without a done marker. Conditionally end a still-owned claim, or keep the host when abandonment remains unresolved.

2. P1 — Recovery downloads permit arbitrary file overwrite on workers. benchmarks/pyathena_bench/fleet.py:594
   Object names are joined directly onto the destination directory without containment checks. An identity restricted to writing the results prefix can place a key ending in attempt-1/../../../../../../pyathena_bench/runner.py; a takeover downloads it over the worker’s executable Python source, which the next orchestrator imports. Such relative segments are valid S3 object keys. Reject absolute paths and traversal, and verify that every resolved destination stays beneath the attempt directory.

3. P2 — Summary upload failures become apparently successful completed jobs. benchmarks/pyathena_bench/fleet.py:1005
   The exception handler now covers summary uploads. If uploading job-summary.csv fails after the runner returned zero, publication continues with exit_code: 0 and summary: null. status() classifies failures only by exit code, so it reports completion without a failure, and workers permanently skip the job despite its missing merged report. Preserve retryability for publication failures or expose report_error as a failed job outcome.

Repairs (cd932b27), all three verified: every lost path releases the claim conditionally (end_lease(claim, "released"), which succeeds only while the claim is still this worker's, including after its own accepted-but-stalled renewal), so the job returns to the fleet; download_tree rejects names that resolve outside the destination; only report generation errors are recorded in the done marker (and status lists them as failed), while a failed summary upload releases the claim and propagates, so another worker publishes the job.

Independent follow-up (relayed Codex result) on cd932b278b8e32355b37da3b336a01afb96c7c1b, thread 01a12f76-684e-7543-8c74-d07e53111809: CLEAN

Covered: 7bbf1e14 → cd932b27; all lost-claim paths, conditional release and takeover protection, worker retirement, download containment, summary publication, status classification, and changed tests.

CLEAN for the requested production-impact criteria. The three reported implementation defects appear resolved; no new qualifying defect found.

The test assessment has one limitation:

• The traversal and summary-upload tests would fail without their respective fixes.
• benchmarks/tests/test_fleet.py:1153 can pass without the abandonment fix. Its mock rejects every renewal, so the stored claim expires and pending_jobs() can reclaim it anyway. To distinguish the original failure, simulate S3 accepting a renewal while its response stalls, leaving the remote claim live after local expiry.

Static review only; no edits, builds, tests, AWS commands, or GitHub access.

Test repair (0e5bbb7d): test_a_claim_lost_only_locally_returns_to_the_fleet now simulates S3 applying each renewal while its response is lost, so the claim stays live remotely; verified that it fails with the release removed (1 failed) and passes with it. Validation on 0e5bbb7d: just benchmark lint, just lint, just docs lint passed; just benchmark test 181 passed, 1 skipped.