Skip to content

Commit b996bf3

Browse files
eordanoclaude
andauthored
fix: harden the LOD manifest-builder lane (#3)
* fix: harden the LOD manifest-builder lane Overwrite read-only files when seeding the manifest-builder workdir (a nix-store copy preserves 444 and poisons every rebuild) and pass the pipeline catalyst explicitly to the npm child via --catalyst, so an inherited CATALYST_URL from the service environment can no longer point scene fetches at the wrong server and emit empty-scene LODs. * fix: rustfmt and clippy compliance Wrap the let-else per rustfmt. Replace set_readonly(false) with an explicit owner-write mode bit on unix (permissions_set_readonly_false is denied by CI clippy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 3d806b9 commit b996bf3

3 files changed

Lines changed: 80 additions & 4 deletions

File tree

crate/src/bin/abgen-lod/main.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,14 @@ fn cmd_placements(argv: &[String]) -> Result<i32> {
394394
.with_context(|| format!("resolve scene {target:?}"))?;
395395
eprintln!("scene entity: {}", ent.entity_id);
396396

397-
let list = acquire_placements(&ent, coords.as_deref(), &iss, manifest_builder, workdir)?;
397+
let list = acquire_placements(
398+
&ent,
399+
coords.as_deref(),
400+
&iss,
401+
manifest_builder,
402+
workdir,
403+
&catalyst,
404+
)?;
398405
println!("{}", serde_json::to_string_pretty(&list)?);
399406
Ok(0)
400407
}
@@ -405,13 +412,15 @@ fn acquire_placements(
405412
iss: &str,
406413
manifest_builder: Option<String>,
407414
workdir: Option<String>,
415+
catalyst: &str,
408416
) -> Result<Vec<placements::Placement>> {
409417
abgen::lodgen::acquire_placements(
410418
ent,
411419
coords,
412420
iss,
413421
manifest_builder.as_deref(),
414422
workdir.map(PathBuf::from).as_deref(),
423+
catalyst,
415424
)
416425
}
417426

@@ -525,6 +534,7 @@ fn cmd_assemble(argv: &[String]) -> Result<i32> {
525534
&iss,
526535
manifest_builder,
527536
workdir,
537+
&catalyst,
528538
)?;
529539
eprintln!("placements: {}", list.len());
530540

crate/src/lodgen/pipeline.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ pub fn acquire_placements(
5555
iss: &str,
5656
manifest_builder: Option<&str>,
5757
workdir: Option<&Path>,
58+
catalyst: &str,
5859
) -> Result<Vec<placements::Placement>> {
5960
let iss_bytes: Option<Vec<u8>> = match iss {
6061
"off" => None,
@@ -108,8 +109,12 @@ pub fn acquire_placements(
108109
PathBuf::from(home).join(".cache/abgen-lod/manifest-builder")
109110
}
110111
};
111-
let Some(manifest_path) =
112-
placements::run_manifest_builder(&run_coords, Path::new(&tool_dir), &work_dir)?
112+
let Some(manifest_path) = placements::run_manifest_builder(
113+
&run_coords,
114+
Path::new(&tool_dir),
115+
&work_dir,
116+
catalyst,
117+
)?
113118
else {
114119
eprintln!(
115120
"manifest-builder: scene ran to completion but emitted no manifest \
@@ -477,6 +482,7 @@ pub fn generate(params: &GenerateParams) -> Result<GenerateOutcome> {
477482
&params.iss,
478483
params.manifest_builder.as_deref(),
479484
mb_workdir.as_deref(),
485+
&params.catalyst,
480486
)?;
481487
let placements_ms = t.elapsed().as_millis();
482488
log.push(format!("placements: {}", placements.len()));

crate/src/lodgen/placements_native.rs

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,18 @@ pub fn fetch_iss(scene_id: &str) -> Result<Option<Vec<u8>>> {
2525
}
2626
}
2727

28+
fn writable(mut perms: std::fs::Permissions) -> std::fs::Permissions {
29+
#[cfg(unix)]
30+
{
31+
use std::os::unix::fs::PermissionsExt;
32+
perms.set_mode(perms.mode() | 0o200);
33+
}
34+
#[cfg(not(unix))]
35+
#[allow(clippy::permissions_set_readonly_false)]
36+
perms.set_readonly(false);
37+
perms
38+
}
39+
2840
fn copy_tree(src: &Path, dst: &Path) -> Result<()> {
2941
std::fs::create_dir_all(dst).with_context(|| format!("mkdir {}", dst.display()))?;
3042
for entry in std::fs::read_dir(src).with_context(|| format!("read dir {}", src.display()))? {
@@ -39,8 +51,21 @@ fn copy_tree(src: &Path, dst: &Path) -> Result<()> {
3951
if entry.file_type()?.is_dir() {
4052
copy_tree(&sp, &dp)?;
4153
} else {
54+
if let Ok(meta) = std::fs::metadata(&dp) {
55+
let perms = meta.permissions();
56+
if perms.readonly() {
57+
let _ = std::fs::set_permissions(&dp, writable(perms));
58+
}
59+
}
4260
std::fs::copy(&sp, &dp)
4361
.with_context(|| format!("copy {} -> {}", sp.display(), dp.display()))?;
62+
let perms = std::fs::metadata(&dp)
63+
.with_context(|| format!("stat {}", dp.display()))?
64+
.permissions();
65+
if perms.readonly() {
66+
std::fs::set_permissions(&dp, writable(perms))
67+
.with_context(|| format!("chmod {}", dp.display()))?;
68+
}
4469
}
4570
}
4671
Ok(())
@@ -73,10 +98,16 @@ pub const MANIFEST_BUILDER_DONE_MARKER: &str = "Finished running frames!";
7398

7499
static MANIFEST_BUILDER_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
75100

101+
pub fn catalyst_base(content_url: &str) -> &str {
102+
let trimmed = content_url.trim_end_matches('/');
103+
trimmed.strip_suffix("/content").unwrap_or(trimmed)
104+
}
105+
76106
pub fn run_manifest_builder(
77107
coords: &str,
78108
tool_dir: &Path,
79109
work_dir: &Path,
110+
catalyst: &str,
80111
) -> Result<Option<PathBuf>> {
81112
let _serialized = MANIFEST_BUILDER_LOCK
82113
.lock()
@@ -101,7 +132,11 @@ pub fn run_manifest_builder(
101132
run_npm(work_dir, &["run", "build"])?;
102133
}
103134
let coords_arg = format!("--coords={coords}");
104-
let stdout = run_npm(work_dir, &["run", "start", &coords_arg, "--overwrite"])?;
135+
let catalyst_arg = format!("--catalyst={}", catalyst_base(catalyst));
136+
let stdout = run_npm(
137+
work_dir,
138+
&["run", "start", &coords_arg, &catalyst_arg, "--overwrite"],
139+
)?;
105140
let out_dir = work_dir.join(MANIFEST_OUTPUT_DIR);
106141
if let Some(rest) = stdout.split("scene id:").nth(1) {
107142
let scene_id: String = rest
@@ -145,3 +180,28 @@ pub fn run_manifest_builder(
145180
),
146181
}
147182
}
183+
184+
#[cfg(test)]
185+
mod tests {
186+
use super::catalyst_base;
187+
188+
#[test]
189+
fn catalyst_base_strips_content_suffix() {
190+
assert_eq!(
191+
catalyst_base("https://peer.decentraland.org/content"),
192+
"https://peer.decentraland.org"
193+
);
194+
assert_eq!(
195+
catalyst_base("https://peer.decentraland.org/content/"),
196+
"https://peer.decentraland.org"
197+
);
198+
assert_eq!(
199+
catalyst_base("http://127.0.0.1:5141"),
200+
"http://127.0.0.1:5141"
201+
);
202+
assert_eq!(
203+
catalyst_base("https://worlds-content-server.decentraland.org/world/name"),
204+
"https://worlds-content-server.decentraland.org/world/name"
205+
);
206+
}
207+
}

0 commit comments

Comments
 (0)