Skip to content

readValidateJSON silently swallows errors, causing crash in concurrent installs #232

@taylortom

Description

@taylortom

Summary

Multiple issues in the CLI's file handling cause crashes and data corruption when installPlugins is called concurrently (e.g. from adapt-authoring-adaptframework during course import).

Problems

1. readValidateJSON silently swallows errors (lib/util/JSONReadValidate.js)

export async function readValidateJSON (filepath, done) {
  try {
    const data = await fs.readFile(filepath, 'utf8')
    validateJSON(data, filepath)
    done?.(null, JSON.parse(data))
    return JSON.parse(data)
  } catch (err) {
    done?.(err.message)
    // BUG: returns undefined instead of re-throwing
  }
}

When the file can't be read (concurrent write, permissions, etc.), this returns undefined instead of throwing, causing downstream TypeError crashes.

2. getManifestDependencies doesn't handle missing data (lib/integration/Project.js:63-66)

async getManifestDependencies () {
  const manifest = await readValidateJSON(this.manifestFilePath)
  return manifest.dependencies  // crashes if manifest is undefined or has no dependencies key
}

3. project.add() has an unsafe read-modify-write cycle (lib/integration/Project.js:123-133)

add (plugin) {
  let manifest = { version: '0.0.0', dependencies: {} }
  if (this.containsManifestFile) {
    manifest = readValidateJSONSync(this.manifestFilePath)  // READ
  }
  manifest.dependencies[plugin.packageName] = ...            // MODIFY
  fs.writeJSONSync(this.manifestFilePath, manifest, ...)     // WRITE
}

Under concurrency, two add() calls can both read the same state, each add their plugin, and the second write silently overwrites the first plugin's entry. This is a classic lost-update race condition.

Impact

When installPlugins is called concurrently, random plugins are reported as "failed to install" even though they installed successfully — it's only the post-install manifest update that fails. Different plugins fail on each run due to non-deterministic timing.

Suggested Fixes

  1. readValidateJSON: re-throw on error

    } catch (err) {
      done?.(err.message)
      throw err
    }
  2. getManifestDependencies: defensive access

    async getManifestDependencies () {
      const manifest = await readValidateJSON(this.manifestFilePath)
      return manifest?.dependencies ?? {}
    }
  3. project.add(): use a file lock or queue writes through a single serialized method to prevent lost updates

Related: adapt-security/adapt-authoring-adaptframework#157

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status

    New

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions