fix(services/webhdfs): encode startAfter and abort the blocks that were written - #8075
Merged
Conversation
…re written
Two independent path defects in this service.
1. The batched lister's startAfter is not encoded.
let mut url = format!(
"{}/webhdfs/v1/{}?op=LISTSTATUS_BATCH",
self.endpoint,
percent_encode_path(&p), // the path is encoded
);
if !start_after.is_empty() {
url += format!("&startAfter={start_after}").as_str(); // this is not
}
start_after is ctx.token, which lister.rs sets verbatim from the last
entry's pathSuffix -- a raw HDFS file name. Measured against
http::Request::get: a name with a space is a hard "invalid uri
character" build error, one with # truncates the marker so the batch
boundary rewinds and a page repeats, and one with & grafts a stray
parameter onto the query.
This is the same defect just fixed for the marker parameter across
obs/swift/cos/azblob/azfile in apache#8073; it was out of that PR's scope only
because it is a format! concatenation rather than a QueryPairsWriter
push. startAfter is the one query value in this file carrying data the
server chose. user.name is config, at eleven sites, and is a separate
question; &{auth} is deliberately a whole query fragment and must stay
verbatim.
2. abort_block deletes paths that were never written.
write_block creates each block at {atomic_write_dir}{block_id} and
complete_block concatenates from the same strings, but abort_block asked
for {block_id} alone. So aborting a multi-block write -- Writer::abort,
or any mid-write failure -- deleted a path that does not exist and left
every uploaded block sitting in atomic_write_dir for ever.
It now resolves atomic_write_dir the same way write_block does, which
also makes the unsupported case explicit rather than deleting a
top-level path named after a UUID.
No new tests: both are single expressions inside async methods whose
seams are an HTTP round trip, and asserting on either would mean
restructuring the URL builder and the writer. The seven existing unit
tests pass, fmt and clippy are clean.
Xuanwo
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
None — found while reading the service. Two independent path defects, both one expression.
1. The batched lister's
startAfteris not encodedwebhdfs_list_status_batchencodes the path and then appends the marker raw:start_afterisctx.token, which the lister sets verbatim from the last entry'spathSuffix— a raw HDFS file name:Measured by feeding the resulting strings to
http::Request::get:report 2024.csvBUILD ERROR: invalid uri character— the listing abortsnote#1.txtstartAfter=note— the rest is a fragment, so the batch boundary rewinds and the page repeatsa&b=cstartAfter=aplus a strayb=cparameterThis is the same defect just fixed for the
markerparameter across obs/swift/cos/azblob/azfile in #8073 — it was outside that PR's scope only because it is aformat!concatenation rather than aQueryPairsWriter::push.startAfteris the one query value in this file that carries data the server chose.user.nameis configuration, appears at eleven sites, and is a separate question;&{auth}is deliberately a whole query fragment and must stay verbatim. Neither is touched here.2.
abort_blockdeletes paths that were never writtenwrite_blockcreates each block under the atomic write dir:and
complete_blockconcatenates from the same strings. Butabort_blockasked for the bare id:So aborting a multi-block write —
Writer::abort(), or any mid-write failure — deleted a path that does not exist and left every uploaded block sitting inatomic_write_dirfor ever. WebHDFS answersDELETEon a missing path with200and{"boolean": false}, so the leak is silent.It now resolves
atomic_write_dirthe same waywrite_blockdoes, which also makes the unsupported case explicit instead of deleting a top-level path named after a UUID.Tests
No new tests, deliberately: both fixes are single expressions inside async methods whose only seam is an HTTP round trip, and asserting on either would mean restructuring the URL builder and the writer — which I would rather not fold into a fix. The 7 existing unit tests pass;
cargo fmt --all -- --checkandcargo clippy -p opendal-service-webhdfs --all-targetsare clean with zero warnings.I kept the two together because they are one-liners in one service; happy to split if you would prefer them reviewed separately.
Are there any user-facing changes?
Yes, for
services-webhdfs: a batched listing whose boundary file name contains a reserved character now resumes correctly instead of failing to build the request or repeating a page, and an aborted multi-block write removes its blocks instead of leaving them behind.