Skip to content

[ruby-compat/01] Guarantee temp table cleanup and fix its ordering vs schema update - #112

Merged
chikamura merged 3 commits into
feature/ruby-compatibilityfrom
ruby-compat/01-temp-table-cleanup-order
Sep 25, 2026
Merged

chikamura merged 3 commits into
feature/ruby-compatibilityfrom
ruby-compat/01-temp-table-cleanup-order

Conversation

@chikamura

Copy link
Copy Markdown
Contributor

Summary

  • Ruby's begin/ensure wraps everything from right after auto_create through the final schema patch, and always deletes the temp table (and cleans up local files) afterward, so cleanup happens even if the load, copy/merge, or schema update fails. The java port only wrapped the copy/merge step, deleted the temp table inline before the schema update, and skipped it if a delete came before an unrelated failure.
  • Wrap the whole post-autoCreate body in try/finally so a failure anywhere (storeCachedSrcFieldsIfNeed, load, copy/merge, updateTableIfNeed) still guarantees the temp table delete, nest a second finally for the local intermediate file cleanup so a delete failure doesn't block it either, and move updateTableIfNeed() before the temp table delete so a delete failure no longer blocks the schema update from being attempted.
  • Updated the existing MockWebServer-based tests to match the new call order (schema update now happens before the temp table delete), and added testRunReplaceModeStillDeletesTempTableWhenStoreCachedSrcFieldsIfNeedFails to cover the temp table delete being guaranteed even when storeCachedSrcFieldsIfNeed() itself fails.

Test plan

  • ./gradlew test (105 tests, 1 skipped, all passing)
  • ./gradlew spotlessApply

Note

Much of the diff is just re-indentation from the new try/finally nesting — reviewing with "Hide whitespace changes" enabled is recommended.

🤖 Generated with Claude Code

@chikamura
chikamura changed the base branch from master to feature/ruby-compatibility September 9, 2026 04:52
@chikamura
chikamura changed the base branch from feature/ruby-compatibility to feature/mockwebserver-e2e-tests September 9, 2026 04:52
Comment on lines +141 to +143
if (task.getTempTable().isPresent()) {
client.deleteTable(task.getTempTable().get());
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the mode switch since it was redundant: BigqueryTaskBuilder.setTempTable() only auto-populates temp_table for exactly these four modes, so task.getTempTable().isPresent() alone already implies the same restriction. This also matches ruby's own unconditional if task['temp_table'] check in its ensure block, with no mode restriction.

https://github.com/trocco-io/embulk-output-bigquery/blob/b4f8d2f4baf1c431a1cee3631790526441c3f9ec/lib/embulk/output/bigquery.rb#L498-L500

@chikamura chikamura changed the title [WIP] Guarantee temp table cleanup and fix its ordering vs schema update [WIP][ruby-compat/01] Guarantee temp table cleanup and fix its ordering vs schema update Sep 15, 2026
Base automatically changed from feature/mockwebserver-e2e-tests to feature/ruby-compatibility September 25, 2026 03:31
chikamura and others added 3 commits September 25, 2026 12:49
Ruby's begin/ensure wraps everything from right after auto_create
through the final schema patch, and always deletes the temp table (and
cleans up local files) afterward, so cleanup happens even if the load,
copy/merge, or schema update fails. The java port only wrapped the
copy/merge step, deleted the temp table inline before the schema
update, and skipped it if a delete came before an unrelated failure.

Wrap the whole post-autoCreate body in try/finally so a failure
anywhere (storeCachedSrcFieldsIfNeed, load, copy/merge,
updateTableIfNeed) still guarantees the temp table delete, nest a
second finally for the local intermediate file cleanup so a delete
failure doesn't block it either, and move updateTableIfNeed() before
the temp table delete so a delete failure no longer blocks the schema
update from being attempted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
transaction() returns right after creating the destination table when
paths.isEmpty(), before ever reaching the load/copy/updateTableIfNeed
logic. Add a regression test covering that the temp table (already
created by autoCreate()) still gets deleted via the guaranteed finally
block in that case, using zero input records to reproduce
paths.isEmpty() (BigqueryPageOutput#add() is never called, so no
writer is registered and no intermediate file is ever created).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eordered cleanup

That test was written against the previous end-of-run order (delete the
temp table, then updateTableIfNeed()'s GET). This branch swaps those
two steps, so its mock responses were being served to the wrong
requests: the GET got a body-less 204 and updateTableIfNeed() NPE'd.
Mirror testRunDeleteInAdvanceMode's order, keeping the initial 404 as
the only difference between the two.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chikamura
chikamura force-pushed the ruby-compat/01-temp-table-cleanup-order branch from 412a48b to ab8e73e Compare September 25, 2026 04:11
@chikamura
chikamura marked this pull request as ready for review September 25, 2026 04:38
@chikamura chikamura changed the title [WIP][ruby-compat/01] Guarantee temp table cleanup and fix its ordering vs schema update [ruby-compat/01] Guarantee temp table cleanup and fix its ordering vs schema update Sep 25, 2026
@chikamura
chikamura requested a review from john-abre September 25, 2026 05:27

@john-abre john-abre left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ありがとうございます!

@chikamura
chikamura merged commit e125110 into feature/ruby-compatibility Sep 25, 2026
@chikamura
chikamura deleted the ruby-compat/01-temp-table-cleanup-order branch September 25, 2026 07:03
chikamura added a commit that referenced this pull request Oct 8, 2026
testRunExpandsStrftimeInTableName (#119) was written against a base that
predates #112, which moved updateTableIfNeed() before the temp table delete.
After the merge the GET table for updateTableIfNeed() received the empty
DELETE response and getTable() returned null (NPE). Order the responses and
request-index assertions like testRunReplaceMode.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 14, 2026
The e2e MockWebServer tests addressed requests as requests.get(n), so every
step that gained or lost a request forced renumbering of everything after
it; that bit when #112 and #119 were merged and again when the load path
became two requests.

Add RequestCursor (next() / assertNoMore()) and walk the requests in order.
Requests referenced twice get a named variable (createTempTable, openUpload,
...). testRunExpandsStrftimeInTableName now also asserts the three requests
it used to skip, since a cursor can't jump over them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 14, 2026
Completeness depended on each test remembering to end with
requests.assertNoMore(); a forgotten call would have silently brought back
the partial-assertion mode in which the strftime test skipped requests
(#112 / #119). RequestCursor is now AutoCloseable and close() does that
check, so tests hold the cursor in a try-with-resources and nothing is
left to remember. A failure in the try body still wins: the close() failure
is attached to it as suppressed.

The harness self-tests for a run that made too many or too few requests
keep the cursor outside a try on purpose, since they assert on the failure
itself (close() naming "GET /two", next() with nothing left).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 14, 2026
The e2e MockWebServer tests addressed requests as requests.get(n), so every
step that gained or lost a request forced renumbering of everything after
it; that bit when #112 and #119 were merged and again when the load path
became two requests.

Add RequestCursor (next() / assertNoMore()) and walk the requests in order.
Requests referenced twice get a named variable (createTempTable, openUpload,
...). testRunExpandsStrftimeInTableName now also asserts the three requests
it used to skip, since a cursor can't jump over them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 15, 2026
The e2e MockWebServer tests addressed requests as requests.get(n), so every
step that gained or lost a request forced renumbering of everything after
it; that bit when #112 and #119 were merged and again when the load path
became two requests.

Add RequestCursor (next() / assertNoMore()) and walk the requests in order.
Requests referenced twice get a named variable (createTempTable, openUpload,
...). testRunExpandsStrftimeInTableName now also asserts the three requests
it used to skip, since a cursor can't jump over them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 15, 2026
Completeness depended on each test remembering to end with
requests.assertNoMore(); a forgotten call would have silently brought back
the partial-assertion mode in which the strftime test skipped requests
(#112 / #119). RequestCursor is now AutoCloseable and close() does that
check, so tests hold the cursor in a try-with-resources and nothing is
left to remember. A failure in the try body still wins: the close() failure
is attached to it as suppressed.

The harness self-tests for a run that made too many or too few requests
keep the cursor outside a try on purpose, since they assert on the failure
itself (close() naming "GET /two", next() with nothing left).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 15, 2026
… index (#126)

* Assert recorded requests through a cursor instead of by index

The e2e MockWebServer tests addressed requests as requests.get(n), so every
step that gained or lost a request forced renumbering of everything after
it; that bit when #112 and #119 were merged and again when the load path
became two requests.

Add RequestCursor (next() / assertNoMore()) and walk the requests in order.
Requests referenced twice get a named variable (createTempTable, openUpload,
...). testRunExpandsStrftimeInTableName now also asserts the three requests
it used to skip, since a cursor can't jump over them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Walk the merged no-compression test with the request cursor

The test came in from the upload-over-http branch in the positional form,
which no longer compiles here since the e2e wrappers return a RequestCursor.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Return a RequestCursor from every runWithMockServer()

The client-level tests still addressed requests by position; now every
runWithMockServer*() hands back a RequestCursor and all MockWebServer tests
walk it, so no test depends on request indices any more. The List + offset
assertLoadAttempt() goes away; the cursor one returns the POST that opened
the upload session, so a test that needs its body (job id, load config)
takes it from the return value instead of expanding the three asserts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Adapt the harness self-test to runWithMockServer() returning a cursor

The body-reread self-test from #127 took the first request with get(0) on
the list runWithMockServer() used to return; with the cursor it is next().

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 15, 2026
Completeness depended on each test remembering to end with
requests.assertNoMore(); a forgotten call would have silently brought back
the partial-assertion mode in which the strftime test skipped requests
(#112 / #119). RequestCursor is now AutoCloseable and close() does that
check, so tests hold the cursor in a try-with-resources and nothing is
left to remember. A failure in the try body still wins: the close() failure
is attached to it as suppressed.

The harness self-tests for a run that made too many or too few requests
keep the cursor outside a try on purpose, since they assert on the failure
itself (close() naming "GET /two", next() with nothing left).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura added a commit that referenced this pull request Oct 15, 2026
* Let the request cursor, not a count assertion, report a diverging run

runWithMockServer() compared the request count to the response count before
the test could walk a single request, so a step that gained or lost a call
failed with a bare "expected:<10> but was:<9>" and the cursor never got to
say where. The count check is gone: the cursor is handed every recorded
request, a run that made fewer calls fails at the first next() with nothing
left, and one that made more fails at assertNoMore(), whose message now lists
the unasserted requests as "GET /path" (the 599 fail-fast answer usually
fails the action before that). The harness self-tests cover both directions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Check that every request was walked when the cursor is closed

Completeness depended on each test remembering to end with
requests.assertNoMore(); a forgotten call would have silently brought back
the partial-assertion mode in which the strftime test skipped requests
(#112 / #119). RequestCursor is now AutoCloseable and close() does that
check, so tests hold the cursor in a try-with-resources and nothing is
left to remember. A failure in the try body still wins: the close() failure
is attached to it as suppressed.

The harness self-tests for a run that made too many or too few requests
keep the cursor outside a try on purpose, since they assert on the failure
itself (close() naming "GET /two", next() with nothing left).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Return both upload requests from assertLoadAttempt()

testRunAppendDirectMode and its no-compression twin re-implemented the
helper inline because it returned only the POST that opens the session,
and they also need the file PUT to check the uploaded bytes. The helper now
returns a LoadAttempt holding both, so those two tests use it as well and a
change to what one attempt consists of (#123) lands in one place. Callers
that only wanted the POST read .openUpload off the result.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Say which request the test asserts when the run recorded fewer

RequestCursor#next() printed position twice; at the point it fails
position equals the recorded count, so the message read "after 9 ... only
9". It now says which request (counting from 1, like the test's response
list) the test is asserting and how many were recorded.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <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.

2 participants